From 2b2c9adef385c064c04bf456fa483dbad742a048 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 14 Jun 2025 17:53:02 -0700 Subject: [PATCH 1/3] Nicer value rendering in API errors Today, if the value passed is a struct, map, or list, we get Go's vative rendering which is clunky. This uses JSON (could be kyaml when that is ready) instead. I hear it already: "But JSON is slow!". I benchmarked it -- for an simple int or string field, JSON is only a little slower (~20%) than a type assertion, but it IS slower, so I left the type assertion in. Remember that this is only called when an API error has occurred. The type assertions do not handle typedefs-to{string, int64, etc} so those will fall back on JSON. Almost all of our errors go thru standard functions which demand string or int64 anyway, so mostly pointless. I also benchmarked using reflect to check `CanInt()` and that is almost exactly as fast as type-switch but handles more cases, so we COULD switch to that instead, if we wanted. I thought it wasn't worth the complexity. JSON is really there to handle composite types. --- .../pkg/util/validation/field/errors.go | 76 +-- .../pkg/util/validation/field/errors_test.go | 482 ++++++++++++++++-- 2 files changed, 468 insertions(+), 90 deletions(-) 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 840d645ee32..39f13e34765 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 @@ -17,8 +17,8 @@ limitations under the License. package field import ( + "encoding/json" "fmt" - "reflect" "strconv" "strings" @@ -72,45 +72,43 @@ var omitValue = OmitValueType{} // for building nice-looking higher-level error reporting. func (e *Error) ErrorBody() string { var s string - switch { - case e.Type == ErrorTypeRequired: + switch e.Type { + case ErrorTypeRequired, ErrorTypeForbidden, ErrorTypeTooLong, ErrorTypeInternal: s = e.Type.String() - case e.Type == ErrorTypeForbidden: - s = e.Type.String() - case e.Type == ErrorTypeTooLong: - s = e.Type.String() - case e.Type == ErrorTypeInternal: - s = e.Type.String() - case e.BadValue == omitValue: - s = e.Type.String() - default: - value := e.BadValue - valueType := reflect.TypeOf(value) - if value == nil || valueType == nil { - value = "null" - } else if valueType.Kind() == reflect.Pointer { - if reflectValue := reflect.ValueOf(value); reflectValue.IsNil() { - value = "null" - } else { - value = reflectValue.Elem().Interface() - } + case ErrorTypeInvalid, ErrorTypeTypeInvalid, ErrorTypeNotSupported, + ErrorTypeNotFound, ErrorTypeDuplicate, ErrorTypeTooMany: + if e.BadValue == omitValue { + s = e.Type.String() + break } - switch t := value.(type) { + switch t := e.BadValue.(type) { case int64, int32, float64, float32, bool: // use simple printer for simple types - s = fmt.Sprintf("%s: %v", e.Type, value) + s = fmt.Sprintf("%s: %v", e.Type, t) case string: s = fmt.Sprintf("%s: %q", e.Type, t) - case fmt.Stringer: - // anything that defines String() is better than raw struct - s = fmt.Sprintf("%s: %s", e.Type, t.String()) default: - // fallback to raw struct - // TODO: internal types have panic guards against json.Marshalling to prevent - // accidental use of internal types in external serialized form. For now, use - // %#v, although it would be better to show a more expressive output in the future - s = fmt.Sprintf("%s: %#v", e.Type, value) + // use more complex techniques to render more complex types + valstr := "" + jb, err := json.Marshal(e.BadValue) + if err == nil { + // best case + valstr = string(jb) + } else if stringer, ok := e.BadValue.(fmt.Stringer); ok { + // anything that defines String() is better than raw struct + valstr = stringer.String() + } else { + // worst case - fallback to raw struct + // TODO: internal types have panic guards against json.Marshalling to prevent + // accidental use of internal types in external serialized form. For now, use + // %#v, although it would be better to show a more expressive output in the future + valstr = fmt.Sprintf("%#v", e.BadValue) + } + s = fmt.Sprintf("%s: %s", e.Type, valstr) } + default: + // NOTE: This panics if we find a code that truly is not supported. + s = e.Type.String() } if len(e.Detail) != 0 { s += fmt.Sprintf(": %s", e.Detail) @@ -258,10 +256,14 @@ func Forbidden(field *Path, detail string) *Error { // 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, it will // be included in the message. The value argument is not used. -func TooLong(field *Path, value interface{}, maxLength int) *Error { +func TooLong(field *Path, _ interface{}, maxLength int) *Error { var msg string if maxLength >= 0 { - msg = fmt.Sprintf("may not be more than %d bytes", maxLength) + bs := "bytes" + if maxLength == 1 { + bs = "byte" + } + msg = fmt.Sprintf("may not be more than %d %s", maxLength, bs) } else { msg = "value is too long" } @@ -281,7 +283,11 @@ func TooMany(field *Path, actualQuantity, maxQuantity int) *Error { var msg string if maxQuantity >= 0 { - msg = fmt.Sprintf("must have at most %d items", maxQuantity) + is := "items" + if maxQuantity == 1 { + is = "item" + } + msg = fmt.Sprintf("must have at most %d %s", maxQuantity, is) } else { msg = "has too many items" } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go index 42b8fe5fc85..2a4f0eb26f1 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go @@ -19,8 +19,10 @@ package field import ( "fmt" "reflect" - "strings" "testing" + "time" + + "k8s.io/utils/ptr" ) func TestMakeFuncs(t *testing.T) { @@ -62,52 +64,6 @@ func TestMakeFuncs(t *testing.T) { } } -func TestErrorUsefulMessage(t *testing.T) { - { - s := Invalid(nil, nil, "").Error() - t.Logf("message: %v", s) - if !strings.Contains(s, "null") { - t.Errorf("error message did not contain 'null': %s", s) - } - } - - s := Invalid(NewPath("foo"), "bar", "deet").Error() - t.Logf("message: %v", s) - for _, part := range []string{"foo", "bar", "deet", ErrorTypeInvalid.String()} { - if !strings.Contains(s, part) { - t.Errorf("error message did not contain expected part '%v'", part) - } - } - - type complicated struct { - Baz int - Qux string - Inner interface{} - KV map[string]int - } - s = Invalid( - NewPath("foo"), - &complicated{ - Baz: 1, - Qux: "aoeu", - Inner: &complicated{Qux: "asdf"}, - KV: map[string]int{"Billy": 2}, - }, - "detail", - ).Error() - t.Logf("message: %v", s) - for _, part := range []string{ - "foo", ErrorTypeInvalid.String(), - "Baz", "Qux", "Inner", "KV", "detail", - "1", "aoeu", "Billy", "2", - // "asdf", TODO: re-enable once we have a better nested printer - } { - if !strings.Contains(s, part) { - t.Errorf("error message did not contain expected part '%v'", part) - } - } -} - func TestToAggregate(t *testing.T) { testCases := struct { ErrList []ErrorList @@ -167,14 +123,6 @@ func TestErrListFilter(t *testing.T) { } } -func TestNotSupported(t *testing.T) { - notSupported := NotSupported(NewPath("f"), "v", []string{"a", "b", "c"}) - expected := `Unsupported value: "v": supported values: "a", "b", "c"` - if notSupported.ErrorBody() != expected { - t.Errorf("Expected: %s\n, but got: %s\n", expected, notSupported.ErrorBody()) - } -} - func TestErrorOrigin(t *testing.T) { err := Invalid(NewPath("field"), "value", "detail") @@ -301,3 +249,427 @@ func TestErrorListRemoveCoveredByDeclarative(t *testing.T) { } } } + +func TestErrorFormatting(t *testing.T) { + cases := []struct { + name string + input *Error + expect string + }{{ + name: "required", + input: &Error{ + Type: ErrorTypeRequired, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Required value: the details`, + }, { + name: "required func", + input: Required(NewPath("path.to.field"), "the details"), + expect: `path.to.field: Required value: the details`, + }, { + name: "forbidden", + input: &Error{ + Type: ErrorTypeForbidden, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Forbidden: the details`, + }, { + name: "forbidden func", + input: Forbidden(NewPath("path.to.field"), "the details"), + expect: `path.to.field: Forbidden: the details`, + }, { + name: "too long", + input: &Error{ + Type: ErrorTypeTooLong, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Too long: the details`, + }, { + name: "too long func(1)", + input: TooLong(NewPath("path.to.field"), "the value", 1), + expect: `path.to.field: Too long: may not be more than 1 byte`, + }, { + name: "too long func(2)", + input: TooLong(NewPath("path.to.field"), "the value", 2), + expect: `path.to.field: Too long: may not be more than 2 bytes`, + }, { + name: "too long func(-1)", + input: TooLong(NewPath("path.to.field"), "the value", -1), + expect: `path.to.field: Too long: value is too long`, + }, { + name: "too many", + input: &Error{ + Type: ErrorTypeTooMany, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Too many: the details`, + }, { + name: "too many func(2, 1)", + input: TooMany(NewPath("path.to.field"), 2, 1), + expect: `path.to.field: Too many: 2: must have at most 1 item`, + }, { + name: "too many func(3, 2)", + input: TooMany(NewPath("path.to.field"), 3, 2), + expect: `path.to.field: Too many: 3: must have at most 2 items`, + }, { + name: "too many func(2, -1)", + input: TooMany(NewPath("path.to.field"), 2, -1), + expect: `path.to.field: Too many: 2: has too many items`, + }, { + name: "too many func(-1, 1)", + input: TooMany(NewPath("path.to.field"), -1, 1), + expect: `path.to.field: Too many: must have at most 1 item`, + }, { + name: "internal error", + input: &Error{ + Type: ErrorTypeInternal, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Internal error: the details`, + }, { + name: "internal error func", + input: InternalError(NewPath("path.to.field"), fmt.Errorf("the error")), + expect: `path.to.field: Internal error: the error`, + }, { + name: "invalid string", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: "the value": the details`, + }, { + name: "invalid string type", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: StringType("the value"), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: "the value": the details`, + }, { + name: "invalid int", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: -42, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: -42: the details`, + }, { + name: "invalid bool", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: true, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: true: the details`, + }, { + name: "invalid struct", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: mkTinyStruct(), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: {"StringField":"stringval","intField":9376,"boolField":true}: the details`, + }, { + name: "invalid list", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: []string{"one", "two", "three"}, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: ["one","two","three"]: the details`, + }, { + name: "invalid map", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: map[string]int{"one": 1, "two": 2, "three": 3}, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: {"one":1,"three":3,"two":2}: the details`, + }, { + name: "invalid time", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: time.Time{}, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: "0001-01-01T00:00:00Z": the details`, + }, { + name: "invalid omitValue", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: omitValue, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: the details`, + }, { + name: "invalid untyped nil", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: nil, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: null: the details`, + }, { + name: "invalid typed nil", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: (*string)(nil), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: null: the details`, + }, { + name: "invalid string ptr", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: ptr.To("the value"), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: "the value": the details`, + }, { + name: "invalid string type ptr", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: ptr.To(StringType("the value")), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: "the value": the details`, + }, { + name: "invalid int ptr", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: ptr.To(-42), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: -42: the details`, + }, { + name: "invalid bool ptr", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: ptr.To(true), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: true: the details`, + }, { + name: "invalid struct ptr", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: ptr.To(mkTinyStruct()), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: {"StringField":"stringval","intField":9376,"boolField":true}: the details`, + }, { + name: "invalid list ptr", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: ptr.To([]string{"one", "two", "three"}), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: ["one","two","three"]: the details`, + }, { + name: "invalid map ptr", + input: &Error{ + Type: ErrorTypeInvalid, + Field: "path.to.field", + BadValue: ptr.To(map[string]int{"one": 1, "two": 2, "three": 3}), + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: {"one":1,"three":3,"two":2}: the details`, + }, { + name: "invalid func", + input: Invalid(NewPath("path.to.field"), "the value", "the details"), + expect: `path.to.field: Invalid value: "the value": the details`, + }, { + name: "not found", + input: &Error{ + Type: ErrorTypeNotFound, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Not found: "the value": the details`, + }, { + name: "not supported", + input: &Error{ + Type: ErrorTypeNotSupported, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Unsupported value: "the value": the details`, + }, { + name: "not supported func", + input: NotSupported(NewPath("path.to.field"), "the value", []string{"val1", "val2"}), + expect: `path.to.field: Unsupported value: "the value": supported values: "val1", "val2"`, + }, { + name: "duplicate", + input: &Error{ + Type: ErrorTypeDuplicate, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Duplicate value: "the value": the details`, + }, { + name: "duplicate func", + input: Duplicate(NewPath("path.to.field"), "the value"), + expect: `path.to.field: Duplicate value: "the value"`, + }, { + name: "type invalid", + input: &Error{ + Type: ErrorTypeTypeInvalid, + Field: "path.to.field", + BadValue: "the value", + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: "the value": the details`, + }, { + name: "type invalid func", + input: TypeInvalid(NewPath("path.to.field"), "the value", "the details"), + expect: `path.to.field: Invalid value: "the value": the details`, + }, { + name: "failed marshal stringer", + input: &Error{ + Type: ErrorTypeTypeInvalid, + Field: "path.to.field", + BadValue: SelfMarshalerStringer{"invisible"}, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: magic: the details`, + }, { + name: "failed marshal non-stringer", + input: &Error{ + Type: ErrorTypeTypeInvalid, + Field: "path.to.field", + BadValue: SelfMarshalerNonStringer{"visible"}, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Invalid value: field.SelfMarshalerNonStringer{S:"visible"}: the details`, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + result := tc.input.Error() + if want := tc.expect; result != want { + t.Errorf("wrong error string:\n expected: %q\n got: %q", want, result) + } + }) + } +} + +type StringType string + +type TinyStruct struct { + StringField string `json:"stringField"` + IntField int `json:"intField"` + BoolField bool `json:"boolField"` +} + +func mkTinyStruct() TinyStruct { + return TinyStruct{ + StringField: "stringval", + IntField: 9376, + BoolField: true, + } +} + +type SelfMarshalerStringer struct{ S string } + +func (SelfMarshalerStringer) MarshalJSON() ([]byte, error) { + return nil, fmt.Errorf("this always fails") +} + +func (SelfMarshalerStringer) String() string { + return "magic" +} + +type SelfMarshalerNonStringer struct{ S string } + +func (SelfMarshalerNonStringer) MarshalJSON() ([]byte, error) { + return nil, fmt.Errorf("this always fails") +} From 4ca91a03052ebf31d373a0de6e12891ae15966b9 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 18 Jun 2025 10:23:01 +0900 Subject: [PATCH 2/3] WIP: Fix tests Notes: * For types that define String() - should we prefer that or JSON? * metav1.Time has a MarshalJSON() and inhereits a String() and they are different * Since validation runs on internal types, we still get some GoNames instead of goNames. --- .../validation/validation_test.go | 24 +++++++++---------- .../validation/validation_test.go | 2 +- .../autoscaling/validation/validation_test.go | 4 ++-- pkg/apis/batch/validation/validation_test.go | 12 +++++----- pkg/credentialprovider/plugin/config.go | 2 +- pkg/credentialprovider/plugin/config_test.go | 4 ++-- .../validation/validation_test.go | 4 ++-- .../pkg/api/validation/objectmeta_test.go | 12 +++++----- .../pkg/util/validation/field/errors_test.go | 6 ++--- .../apiserver/validation/validation_test.go | 2 +- .../logs/api/v1/validate_test.go | 4 ++-- 11 files changed, 38 insertions(+), 38 deletions(-) diff --git a/pkg/apis/admissionregistration/validation/validation_test.go b/pkg/apis/admissionregistration/validation/validation_test.go index 85dc825583a..2fe7f3a507f 100644 --- a/pkg/apis/admissionregistration/validation/validation_test.go +++ b/pkg/apis/admissionregistration/validation/validation_test.go @@ -107,7 +107,7 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { AdmissionReviewVersions: []string{"invalidVersion"}, }, }, true), - expectedError: `Invalid value: []string{"invalidVersion"}`, + expectedError: `Invalid value: ["invalidVersion"]`, }, { name: "should fail on duplicate AdmissionReviewVersion", config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{{ @@ -313,7 +313,7 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) { }}, }, }, true), - expectedError: `webhooks[0].rules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + expectedError: `webhooks[0].rules[0].resources: Invalid value: ["*/*","a"]: if '*/*' is present, must not specify other resources`, }, { name: "FailurePolicy can only be \"Ignore\" or \"Fail\"", config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{{ @@ -892,7 +892,7 @@ func TestValidateValidatingWebhookConfigurationUpdate(t *testing.T) { AdmissionReviewVersions: []string{"v1beta1", "invalid-v1"}, }, }, true), - expectedError: `Invalid value: []string{"invalid-v1"}`, + expectedError: `Invalid value: ["invalid-v1"]`, }, { name: "should fail on invalid AdmissionReviewVersion with missing previous versions", config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{{ @@ -908,7 +908,7 @@ func TestValidateValidatingWebhookConfigurationUpdate(t *testing.T) { SideEffects: &unknownSideEffect, }, }, false), - expectedError: `Invalid value: []string{"invalid-v1"}`, + expectedError: `Invalid value: ["invalid-v1"]`, }, { name: "Webhooks must have unique names when old config has unique names", config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{{ @@ -1084,7 +1084,7 @@ func TestValidateMutatingWebhookConfiguration(t *testing.T) { AdmissionReviewVersions: []string{"invalidVersion"}, }, }, true), - expectedError: `Invalid value: []string{"invalidVersion"}`, + expectedError: `Invalid value: ["invalidVersion"]`, }, { name: "should fail on duplicate AdmissionReviewVersion", config: newMutatingWebhookConfiguration([]admissionregistration.MutatingWebhook{{ @@ -1290,7 +1290,7 @@ func TestValidateMutatingWebhookConfiguration(t *testing.T) { }}, }, }, true), - expectedError: `webhooks[0].rules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + expectedError: `webhooks[0].rules[0].resources: Invalid value: ["*/*","a"]: if '*/*' is present, must not specify other resources`, }, { name: "FailurePolicy can only be \"Ignore\" or \"Fail\"", config: newMutatingWebhookConfiguration([]admissionregistration.MutatingWebhook{{ @@ -1885,7 +1885,7 @@ func TestValidateMutatingWebhookConfigurationUpdate(t *testing.T) { AdmissionReviewVersions: []string{"v1beta1", "invalid-v1"}, }, }, true), - expectedError: `Invalid value: []string{"invalid-v1"}`, + expectedError: `Invalid value: ["invalid-v1"]`, }, { name: "should fail on invalid AdmissionReviewVersion with missing previous versions", config: newMutatingWebhookConfiguration([]admissionregistration.MutatingWebhook{{ @@ -1901,7 +1901,7 @@ func TestValidateMutatingWebhookConfigurationUpdate(t *testing.T) { SideEffects: &unknownSideEffect, }, }, false), - expectedError: `Invalid value: []string{"invalid-v1"}`, + expectedError: `Invalid value: ["invalid-v1"]`, }, { name: "Webhooks can have duplicate names when old config has duplicate names", config: newMutatingWebhookConfiguration([]admissionregistration.MutatingWebhook{{ @@ -2592,7 +2592,7 @@ func TestValidateValidatingAdmissionPolicy(t *testing.T) { }, }, }, - expectedError: `spec.matchConstraints.resourceRules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + expectedError: `spec.matchConstraints.resourceRules[0].resources: Invalid value: ["*/*","a"]: if '*/*' is present, must not specify other resources`, }, { name: "invalid expression", config: &admissionregistration.ValidatingAdmissionPolicy{ @@ -3900,7 +3900,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { }, }, }, - expectedError: `spec.matchResources.resourceRules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + expectedError: `spec.matchResources.resourceRules[0].resources: Invalid value: ["*/*","a"]: if '*/*' is present, must not specify other resources`, }, { name: "validationActions must be unique", config: &admissionregistration.ValidatingAdmissionPolicyBinding{ @@ -5055,7 +5055,7 @@ func TestValidateMutatingAdmissionPolicy(t *testing.T) { }, }, }, - expectedError: `spec.matchConstraints.resourceRules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + expectedError: `spec.matchConstraints.resourceRules[0].resources: Invalid value: ["*/*","a"]: if '*/*' is present, must not specify other resources`, }, { name: "patchType required", config: &admissionregistration.MutatingAdmissionPolicy{ @@ -5946,7 +5946,7 @@ func TestValidateMutatingAdmissionPolicyBinding(t *testing.T) { }, }, }, - expectedError: `spec.matchResources.resourceRules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + expectedError: `spec.matchResources.resourceRules[0].resources: Invalid value: ["*/*","a"]: if '*/*' is present, must not specify other resources`, }, { name: "paramRef selector must not be set when name is set", config: &admissionregistration.MutatingAdmissionPolicyBinding{ diff --git a/pkg/apis/apiserverinternal/validation/validation_test.go b/pkg/apis/apiserverinternal/validation/validation_test.go index 4fd668444c1..b62fc9eb608 100644 --- a/pkg/apis/apiserverinternal/validation/validation_test.go +++ b/pkg/apis/apiserverinternal/validation/validation_test.go @@ -280,7 +280,7 @@ func TestValidateCommonVersion(t *testing.T) { }}, CommonEncodingVersion: nil, }, - expectedErr: "Invalid value: \"null\": the common encoding version is v1alpha1", + expectedErr: "Invalid value: null: the common encoding version is v1alpha1", }, { status: apiserverinternal.StorageVersionStatus{ StorageVersions: []apiserverinternal.ServerStorageVersion{{ diff --git a/pkg/apis/autoscaling/validation/validation_test.go b/pkg/apis/autoscaling/validation/validation_test.go index aeac3c46381..bd0f53e66ea 100644 --- a/pkg/apis/autoscaling/validation/validation_test.go +++ b/pkg/apis/autoscaling/validation/validation_test.go @@ -927,7 +927,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { }}, }, }, - msg: "Invalid value: InvalidResource: must be a standard resource type or fully qualified", + msg: "Invalid value: \"InvalidResource\": must be a standard resource type or fully qualified", }, { horizontalPodAutoscaler: autoscaling.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{Name: "myautoscaler", Namespace: metav1.NamespaceDefault}, @@ -1429,7 +1429,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { if len(errs) == 0 { t.Errorf("expected failure for %q", c.msg) } else if !strings.Contains(errs[0].Error(), c.msg) { - t.Errorf("unexpected error: %q, expected: %q", errs[0], c.msg) + t.Errorf("unexpected error:\n expected: %q\n got: %q", c.msg, errs[0]) } } diff --git a/pkg/apis/batch/validation/validation_test.go b/pkg/apis/batch/validation/validation_test.go index 062ee6b9fd8..2e30d0e1998 100644 --- a/pkg/apis/batch/validation/validation_test.go +++ b/pkg/apis/batch/validation/validation_test.go @@ -458,7 +458,7 @@ func TestValidateJob(t *testing.T) { }, }, }, - `spec.successPolicy: Invalid value: batch.SuccessPolicy{Rules:[]batch.SuccessPolicyRule{}}: requires indexed completion mode`: { + `spec.successPolicy: Invalid value: {"Rules":[]}: requires indexed completion mode`: { job: batch.Job{ ObjectMeta: validJobObjectMeta, Spec: batch.JobSpec{ @@ -739,7 +739,7 @@ func TestValidateJob(t *testing.T) { }, opts: JobValidationOptions{RequirePrefixedLabels: true}, }, - `spec.podFailurePolicy.rules[0].onExitCodes.values: Invalid value: []int32{19, 11}: must be ordered`: { + `spec.podFailurePolicy.rules[0].onExitCodes.values: Invalid value: [19,11]: must be ordered`: { job: batch.Job{ ObjectMeta: validJobObjectMeta, Spec: batch.JobSpec{ @@ -758,7 +758,7 @@ func TestValidateJob(t *testing.T) { }, opts: JobValidationOptions{RequirePrefixedLabels: true}, }, - `spec.podFailurePolicy.rules[0].onExitCodes.values: Invalid value: []int32{}: at least one value is required`: { + `spec.podFailurePolicy.rules[0].onExitCodes.values: Invalid value: []: at least one value is required`: { job: batch.Job{ ObjectMeta: validJobObjectMeta, Spec: batch.JobSpec{ @@ -1239,7 +1239,7 @@ func TestValidateJob(t *testing.T) { }, opts: JobValidationOptions{RequirePrefixedLabels: true}, }, - "spec.template.metadata.labels: Invalid value: map[string]string{\"y\":\"z\"}: `selector` does not match template `labels`": { + "spec.template.metadata.labels: Invalid value: {\"y\":\"z\"}: `selector` does not match template `labels`": { job: batch.Job{ ObjectMeta: metav1.ObjectMeta{ Name: "myjob", @@ -1259,7 +1259,7 @@ func TestValidateJob(t *testing.T) { }, opts: JobValidationOptions{RequirePrefixedLabels: true}, }, - "spec.template.metadata.labels: Invalid value: map[string]string{\"controller-uid\":\"4d5e6f\"}: `selector` does not match template `labels`": { + "spec.template.metadata.labels: Invalid value: {\"controller-uid\":\"4d5e6f\"}: `selector` does not match template `labels`": { job: batch.Job{ ObjectMeta: metav1.ObjectMeta{ Name: "myjob", @@ -1407,7 +1407,7 @@ func TestValidateJob(t *testing.T) { }, opts: JobValidationOptions{}, }, - "spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{\"a\":\"b\"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: `selector` not auto-generated": { + "spec.selector: Invalid value: {\"matchLabels\":{\"a\":\"b\"}}: `selector` not auto-generated": { job: batch.Job{ ObjectMeta: metav1.ObjectMeta{ Name: "myjob", diff --git a/pkg/credentialprovider/plugin/config.go b/pkg/credentialprovider/plugin/config.go index 1f6f441c7e7..49ba18a43d0 100644 --- a/pkg/credentialprovider/plugin/config.go +++ b/pkg/credentialprovider/plugin/config.go @@ -125,7 +125,7 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo } if provider.DefaultCacheDuration != nil && provider.DefaultCacheDuration.Duration < 0 { - allErrs = append(allErrs, field.Invalid(fieldPath.Child("defaultCacheDuration"), provider.DefaultCacheDuration.Duration, "defaultCacheDuration must be greater than or equal to 0")) + allErrs = append(allErrs, field.Invalid(fieldPath.Child("defaultCacheDuration"), provider.DefaultCacheDuration, "defaultCacheDuration must be greater than or equal to 0")) } if provider.TokenAttributes != nil { diff --git a/pkg/credentialprovider/plugin/config_test.go b/pkg/credentialprovider/plugin/config_test.go index c0cefc65183..dafc29ecc07 100644 --- a/pkg/credentialprovider/plugin/config_test.go +++ b/pkg/credentialprovider/plugin/config_test.go @@ -570,7 +570,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) { }, }, }, - expectErr: "providers.defaultCacheDuration: Invalid value: -1m0s: defaultCacheDuration must be greater than or equal to 0", + expectErr: "providers.defaultCacheDuration: Invalid value: \"-1m0s\": defaultCacheDuration must be greater than or equal to 0", }, { name: "invalid match image", @@ -754,7 +754,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) { }, }, saTokenForCredentialProviders: true, - expectErr: `providers.tokenAttributes: Invalid value: []string{"now-with-dashes/simple-2"}: annotation keys cannot be both required and optional`, + expectErr: `providers.tokenAttributes: Invalid value: ["now-with-dashes/simple-2"]: annotation keys cannot be both required and optional`, }, { name: "required annotation keys set when requireServiceAccount is false", 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 7662a613069..bd60b6493ec 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 @@ -10987,7 +10987,7 @@ func TestValidateCustomResourceDefinitionStoredVersions(t *testing.T) { storageVersion: "v1", storedVersions: []string{}, errors: []validationMatch{ - invalid("status", "storedVersions").contains("Invalid value: []string{}: must have at least one stored version"), + invalid("status", "storedVersions").contains("Invalid value: []: must have at least one stored version"), }, }, { @@ -11011,7 +11011,7 @@ func TestValidateCustomResourceDefinitionStoredVersions(t *testing.T) { storageVersion: "v1", storedVersions: []string{"v1alpha", "v1beta1"}, errors: []validationMatch{ - invalid("status", "storedVersions").contains("Invalid value: []string{\"v1alpha\", \"v1beta1\"}: must have the storage version v1"), + invalid("status", "storedVersions").contains("Invalid value: [\"v1alpha\",\"v1beta1\"]: must have the storage version v1"), }, }, } diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go b/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go index 0331cf9f3b9..e4d50f41e12 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta_test.go @@ -328,19 +328,19 @@ func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, - ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: 1970-01-01 00:16:40 +0000 UTC: field is immutable"}, + ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"1970-01-01T00:16:40Z\": field is immutable"}, }, "invalid clear deletionTimestamp": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, - ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"null\": field is immutable"}, + ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: null: field is immutable"}, }, "invalid change deletionTimestamp": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &later}, ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &later}, - ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: 1970-01-01 00:33:20 +0000 UTC: field is immutable"}, + ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"1970-01-01T00:33:20Z\": field is immutable"}, }, "invalid set deletionGracePeriodSeconds": { @@ -353,7 +353,7 @@ func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort}, New: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, ExpectedNew: metav1.ObjectMeta{Name: "test", ResourceVersion: "1"}, - ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: \"null\": field is immutable"}, + ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: null: field is immutable"}, }, "invalid change deletionGracePeriodSeconds": { Old: metav1.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort}, @@ -373,7 +373,7 @@ func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) { } for i := range errs { if errs[i].Error() != tc.ExpectedErrs[i] { - t.Errorf("%s: error #%d: expected %q, got %q", k, i, tc.ExpectedErrs[i], errs[i].Error()) + t.Errorf("%s: error #%d:\n expected: %q\n got: %q", k, i, tc.ExpectedErrs[i], errs[i].Error()) } } if !reflect.DeepEqual(tc.New, tc.ExpectedNew) { @@ -419,7 +419,7 @@ func TestObjectMetaGenerationUpdate(t *testing.T) { } for i := range errList { if errList[i] != tc.ExpectedErrs[i] { - t.Errorf("%s: error #%d: expected %q, got %q", k, i, tc.ExpectedErrs[i], errList[i]) + t.Errorf("%s: error #%d:\n expected: %q\n got: %q", k, i, tc.ExpectedErrs[i], errs[i].Error()) } } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go index 2a4f0eb26f1..a1b57c1c5f7 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go @@ -318,7 +318,7 @@ func TestErrorFormatting(t *testing.T) { Origin: "theOrigin", CoveredByDeclarative: true, }, - expect: `path.to.field: Too many: the details`, + expect: `path.to.field: Too many: "the value": the details`, }, { name: "too many func(2, 1)", input: TooMany(NewPath("path.to.field"), 2, 1), @@ -404,7 +404,7 @@ func TestErrorFormatting(t *testing.T) { Origin: "theOrigin", CoveredByDeclarative: true, }, - expect: `path.to.field: Invalid value: {"StringField":"stringval","intField":9376,"boolField":true}: the details`, + expect: `path.to.field: Invalid value: {"stringField":"stringval","intField":9376,"boolField":true}: the details`, }, { name: "invalid list", input: &Error{ @@ -525,7 +525,7 @@ func TestErrorFormatting(t *testing.T) { Origin: "theOrigin", CoveredByDeclarative: true, }, - expect: `path.to.field: Invalid value: {"StringField":"stringval","intField":9376,"boolField":true}: the details`, + expect: `path.to.field: Invalid value: {"stringField":"stringval","intField":9376,"boolField":true}: the details`, }, { name: "invalid list ptr", input: &Error{ diff --git a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go index 6d761b59318..97fcc82de29 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go @@ -905,7 +905,7 @@ func TestValidateAudiences(t *testing.T) { name: "multiple audiences set when structured authn feature is disabled", in: []string{"audience1", "audience2"}, matchPolicy: "MatchAny", - want: `issuer.audiences: Invalid value: []string{"audience1", "audience2"}: multiple audiences are not supported when StructuredAuthenticationConfiguration feature gate is disabled`, + want: `issuer.audiences: Invalid value: ["audience1","audience2"]: multiple audiences are not supported when StructuredAuthenticationConfiguration feature gate is disabled`, }, } diff --git a/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go b/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go index 3d1955b0b2a..956f225c232 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/validate_test.go @@ -77,7 +77,7 @@ func TestValidation(t *testing.T) { Format: "text", Verbosity: math.MaxInt32 + 1, }, - expectErrors: `verbosity: Invalid value: 0x80000000: Must be <= 2147483647`, + expectErrors: `verbosity: Invalid value: 2147483648: Must be <= 2147483647`, }, "vmodule-verbosity-overflow": { config: LoggingConfiguration{ @@ -89,7 +89,7 @@ func TestValidation(t *testing.T) { }, }, }, - expectErrors: `vmodule[0]: Invalid value: 0x80000000: Must be <= 2147483647`, + expectErrors: `vmodule[0]: Invalid value: 2147483648: Must be <= 2147483647`, }, "vmodule-empty-pattern": { config: LoggingConfiguration{ From e68d601344965e67fc09da9d7e4979c4bf72c9c3 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 14 Jun 2025 18:12:36 -0700 Subject: [PATCH 3/3] Don't panic in case of an unknown API error code --- .../apimachinery/pkg/util/validation/field/errors.go | 6 +++--- .../pkg/util/validation/field/errors_test.go | 11 +++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) 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 39f13e34765..f2a983aebf6 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 @@ -107,8 +107,8 @@ func (e *Error) ErrorBody() string { s = fmt.Sprintf("%s: %s", e.Type, valstr) } default: - // NOTE: This panics if we find a code that truly is not supported. - s = e.Type.String() + internal := InternalError(nil, fmt.Errorf("unhandled error code: %s: please report this", e.Type)) + s = internal.ErrorBody() } if len(e.Detail) != 0 { s += fmt.Sprintf(": %s", e.Detail) @@ -195,7 +195,7 @@ func (t ErrorType) String() string { case ErrorTypeTypeInvalid: return "Invalid value" default: - panic(fmt.Sprintf("unrecognized validation error: %q", string(t))) + return fmt.Sprintf("", string(t)) } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go index a1b57c1c5f7..a8c7052f9fb 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/errors_test.go @@ -630,6 +630,17 @@ func TestErrorFormatting(t *testing.T) { CoveredByDeclarative: true, }, expect: `path.to.field: Invalid value: field.SelfMarshalerNonStringer{S:"visible"}: the details`, + }, { + name: "unknown error type", + input: &Error{ + Type: "not real", + Field: "path.to.field", + BadValue: SelfMarshalerNonStringer{"visible"}, + Detail: "the details", + Origin: "theOrigin", + CoveredByDeclarative: true, + }, + expect: `path.to.field: Internal error: unhandled error code: : please report this: the details`, }} for _, tc := range cases {