From 50795b1afad70d831e99e12b694da88fcb454143 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 24 Mar 2022 10:47:24 -0700 Subject: [PATCH] Update err handling --- .../pkg/apiserver/validation/validation.go | 25 +++++++--- .../apiserver/validation/validation_test.go | 49 ++++++++++++++++++- .../pkg/util/validation/field/errors.go | 47 ++++++++++++++++-- 3 files changed, 109 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go index ed999145830..30c7264c824 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go @@ -84,14 +84,25 @@ func ValidateCustomResource(fldPath *field.Path, customResource interface{}, val if err.Value != nil { value = err.Value } - allErrs = append(allErrs, field.TooLongFail(errPath, value, err.Error())) - - case openapierrors.TooManyPropertiesCode, openapierrors.MaxItemsFailCode: - value := interface{}("") - if err.Value != nil { - value = err.Value + max := int64(-1) + if i, ok := err.Value.(int64); ok { + max = i } - allErrs = append(allErrs, field.TooManyFail(errPath, value, err.Error())) + allErrs = append(allErrs, field.TooLongMaxLength(errPath, value, int(max))) + + case openapierrors.MaxItemsFailCode: + max := int64(-1) + if i, ok := err.Value.(int64); ok { + max = i + } + allErrs = append(allErrs, field.TooMany(errPath, -1, int(max))) + + case openapierrors.TooManyPropertiesCode: + max := int64(-1) + if i, ok := err.Value.(int64); ok { + max = i + } + allErrs = append(allErrs, field.TooMany(errPath, -1, int(max))) case openapierrors.InvalidTypeCode: value := interface{}("") diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go index b27bd251c30..8cad7f88080 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation_test.go @@ -26,10 +26,9 @@ import ( "github.com/google/go-cmp/cmp" + utilpointer "k8s.io/utils/pointer" kjson "sigs.k8s.io/json" - kubeopenapispec "k8s.io/kube-openapi/pkg/validation/spec" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsfuzzer "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/fuzzer" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -41,6 +40,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" + kubeopenapispec "k8s.io/kube-openapi/pkg/validation/spec" ) // TestRoundTrip checks the conversion to go-openapi types. @@ -535,6 +535,51 @@ func TestValidateCustomResource(t *testing.T) { }}, }, }, + {name: "maxProperties", + schema: apiextensions.JSONSchemaProps{ + Properties: map[string]apiextensions.JSONSchemaProps{ + "fieldX": { + Type: "object", + MaxProperties: utilpointer.Int64(2), + }, + }, + }, + failingObjects: []failingObject{ + {object: map[string]interface{}{"fieldX": map[string]interface{}{"a": true, "b": true, "c": true}}, expectErrs: []string{ + `fieldX: Too many: must have at most 2 items`, + }}, + }, + }, + {name: "maxItems", + schema: apiextensions.JSONSchemaProps{ + Properties: map[string]apiextensions.JSONSchemaProps{ + "fieldX": { + Type: "array", + MaxItems: utilpointer.Int64(2), + }, + }, + }, + failingObjects: []failingObject{ + {object: map[string]interface{}{"fieldX": []interface{}{"a", "b", "c"}}, expectErrs: []string{ + `fieldX: Too many: must have at most 3 items`, + }}, + }, + }, + {name: "maxLength", + schema: apiextensions.JSONSchemaProps{ + Properties: map[string]apiextensions.JSONSchemaProps{ + "fieldX": { + Type: "string", + MaxLength: utilpointer.Int64(2), + }, + }, + }, + failingObjects: []failingObject{ + {object: map[string]interface{}{"fieldX": "abc"}, expectErrs: []string{ + `fieldX: Too long: value is too long`, + }}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors.go index c54c58ce11d..0cb90ab79a2 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors.go @@ -42,12 +42,24 @@ func (v *Error) Error() string { return fmt.Sprintf("%s: %s", v.Field, v.ErrorBody()) } +type omitValueType struct{} + +var omitValue = omitValueType{} + // ErrorBody returns the error message without the field name. This is useful // for building nice-looking higher-level error reporting. func (v *Error) ErrorBody() string { var s string - switch v.Type { - case ErrorTypeRequired, ErrorTypeForbidden, ErrorTypeTooLong, ErrorTypeInternal: + switch { + case v.Type == ErrorTypeRequired: + s = v.Type.String() + case v.Type == ErrorTypeForbidden: + s = v.Type.String() + case v.Type == ErrorTypeTooLong: + s = v.Type.String() + case v.Type == ErrorTypeInternal: + s = v.Type.String() + case v.BadValue == omitValue: s = v.Type.String() default: value := v.BadValue @@ -226,11 +238,40 @@ func TooLong(field *Path, value interface{}, maxLength int) *Error { return &Error{ErrorTypeTooLong, field.String(), value, fmt.Sprintf("must have at most %d bytes", maxLength)} } +// TooLongMaxLength returns a *Error indicating "too long". This is used to +// report that the given value is too long. This is similar to +// Invalid, but the returned error will not include the too-long +// value. If maxLength is negative, no max length will be included in the message. +func TooLongMaxLength(field *Path, value interface{}, maxLength int) *Error { + var msg string + if maxLength >= 0 { + msg = fmt.Sprintf("may not be longer than %d", maxLength) + } else { + msg = "value is too long" + } + return &Error{ErrorTypeTooLong, field.String(), value, msg} +} + // TooMany returns a *Error indicating "too many". This is used to // report that a given list has too many items. This is similar to TooLong, // but the returned error indicates quantity instead of length. func TooMany(field *Path, actualQuantity, maxQuantity int) *Error { - return &Error{ErrorTypeTooMany, field.String(), actualQuantity, fmt.Sprintf("must have at most %d items", maxQuantity)} + var msg string + + if maxQuantity >= 0 { + msg = fmt.Sprintf("must have at most %d items", maxQuantity) + } else { + msg = "has too many items" + } + + var actual interface{} + if actualQuantity >= 0 { + actual = actualQuantity + } else { + actual = omitValue + } + + return &Error{ErrorTypeTooMany, field.String(), actual, msg} } // InternalError returns a *Error indicating "internal error". This is used