diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go index a25da98e3fd..96811ca57c5 100644 --- a/pkg/apis/apps/validation/validation_test.go +++ b/pkg/apis/apps/validation/validation_test.go @@ -523,7 +523,7 @@ func TestValidateStatefulSet(t *testing.T) { }, } - cmpOpts := []cmp.Option{cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail"), cmpopts.SortSlices(func(a, b *field.Error) bool { return a.Error() < b.Error() })} + cmpOpts := []cmp.Option{cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail", "Origin"), cmpopts.SortSlices(func(a, b *field.Error) bool { return a.Error() < b.Error() })} for _, testCase := range append(successCases, errorCases...) { name := testCase.name var testTitle string @@ -936,7 +936,7 @@ func TestValidateStatefulSetUpdate(t *testing.T) { } cmpOpts := []cmp.Option{ - cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail"), + cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail", "Origin"), cmpopts.SortSlices(func(a, b *field.Error) bool { return a.Error() < b.Error() }), cmpopts.EquateEmpty(), } diff --git a/pkg/apis/batch/validation/validation_test.go b/pkg/apis/batch/validation/validation_test.go index 3f23cf0ae3c..062ee6b9fd8 100644 --- a/pkg/apis/batch/validation/validation_test.go +++ b/pkg/apis/batch/validation/validation_test.go @@ -48,7 +48,7 @@ var ( timeZoneEmptySpace = " " ) -var ignoreErrValueDetail = cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail") +var ignoreErrValueDetail = cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail", "Origin") func getValidManualSelector() *metav1.LabelSelector { return &metav1.LabelSelector{ diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 231959c1d24..81c5a4fb4fc 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -350,6 +350,15 @@ func ValidateNonnegativeQuantity(value resource.Quantity, fldPath *field.Path) f return allErrs } +// Validates that given value is positive. +func ValidatePositiveField(value int64, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if value <= 0 { + allErrs = append(allErrs, field.Invalid(fldPath, value, isNotPositiveErrorMsg).WithOrigin("minimum")) + } + return allErrs +} + // Validates that a Quantity is positive func ValidatePositiveQuantityValue(value resource.Quantity, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} @@ -7905,8 +7914,8 @@ func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstrai for i, constraint := range constraints { subFldPath := fldPath.Index(i) - if err := ValidateMaxSkew(subFldPath.Child("maxSkew"), constraint.MaxSkew); err != nil { - allErrs = append(allErrs, err) + if errs := ValidatePositiveField(int64(constraint.MaxSkew), subFldPath.Child("maxSkew")); len(errs) > 0 { + allErrs = append(allErrs, errs...) } if err := ValidateTopologyKey(subFldPath.Child("topologyKey"), constraint.TopologyKey); err != nil { allErrs = append(allErrs, err) @@ -7934,26 +7943,16 @@ func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstrai return allErrs } -// ValidateMaxSkew tests that the argument is a valid MaxSkew. -func ValidateMaxSkew(fldPath *field.Path, maxSkew int32) *field.Error { - if maxSkew <= 0 { - return field.Invalid(fldPath, maxSkew, isNotPositiveErrorMsg) - } - return nil -} - // validateMinDomains tests that the argument is a valid MinDomains. func validateMinDomains(fldPath *field.Path, minDomains *int32, action core.UnsatisfiableConstraintAction) field.ErrorList { if minDomains == nil { return nil } var allErrs field.ErrorList - if *minDomains <= 0 { - allErrs = append(allErrs, field.Invalid(fldPath, minDomains, isNotPositiveErrorMsg)) - } + allErrs = append(allErrs, ValidatePositiveField(int64(*minDomains), fldPath)...) // When MinDomains is non-nil, whenUnsatisfiable must be DoNotSchedule. if action != core.DoNotSchedule { - allErrs = append(allErrs, field.Invalid(fldPath, minDomains, fmt.Sprintf("can only use minDomains if whenUnsatisfiable=%s, not %s", core.DoNotSchedule, action))) + allErrs = append(allErrs, field.Invalid(fldPath, minDomains, fmt.Sprintf("can only use minDomains if whenUnsatisfiable=%s, not %s", core.DoNotSchedule, action)).WithOrigin("dependsOn")) } return allErrs } @@ -8077,7 +8076,7 @@ func validateMatchLabelKeysInTopologySpread(fldPath *field.Path, matchLabelKeys for i, key := range matchLabelKeys { allErrs = append(allErrs, unversionedvalidation.ValidateLabelName(key, fldPath.Index(i))...) if labelSelectorKeys.Has(key) { - allErrs = append(allErrs, field.Invalid(fldPath.Index(i), key, "exists in both matchLabelKeys and labelSelector")) + allErrs = append(allErrs, field.Invalid(fldPath.Index(i), key, "exists in both matchLabelKeys and labelSelector").WithOrigin("overlappingKeys")) } } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index f269cbaf96b..5d4ee61447e 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" + fldtest "k8s.io/apimachinery/pkg/util/validation/field/testing" "k8s.io/apimachinery/pkg/util/version" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/component-base/featuregate" @@ -20717,25 +20718,32 @@ func TestValidateEndpointsCreate(t *testing.T) { } errorCases := map[string]struct { - endpoints core.Endpoints - errorType field.ErrorType - errorOrigin string + endpoints core.Endpoints + expectedErrs field.ErrorList }{ "missing namespace": { endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "mysvc"}}, - errorType: "FieldValueRequired", + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("metadata.namespace"), ""), + }, }, "missing name": { endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Namespace: "namespace"}}, - errorType: "FieldValueRequired", + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("metadata.name"), ""), + }, }, "invalid namespace": { endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "no@#invalid.;chars\"allowed"}}, - errorType: "FieldValueInvalid", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("metadata.namespace"), nil, ""), + }, }, "invalid name": { endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "-_Invliad^&Characters", Namespace: "namespace"}}, - errorType: "FieldValueInvalid", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("metadata.name"), nil, ""), + }, }, "empty addresses": { endpoints: core.Endpoints{ @@ -20744,7 +20752,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "a", Port: 93, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueRequired", + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("subsets[0]"), ""), + }, }, "invalid IP": { endpoints: core.Endpoints{ @@ -20754,8 +20764,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "a", Port: 93, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "format=ip-sloppy", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=ip-sloppy"), + }, }, "Multiple ports, one without name": { endpoints: core.Endpoints{ @@ -20765,7 +20776,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Port: 8675, Protocol: "TCP"}, {Name: "b", Port: 309, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueRequired", + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("subsets[0].ports[0].name"), ""), + }, }, "Invalid port number": { endpoints: core.Endpoints{ @@ -20775,8 +20788,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "a", Port: 66000, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "portNum", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].ports[0].port"), nil, "").WithOrigin("portNum"), + }, }, "Invalid protocol": { endpoints: core.Endpoints{ @@ -20786,7 +20800,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "a", Port: 93, Protocol: "Protocol"}}, }}, }, - errorType: "FieldValueNotSupported", + expectedErrs: field.ErrorList{ + field.NotSupported[string](field.NewPath("subsets[0].ports[0].protocol"), nil, nil), + }, }, "Address missing IP": { endpoints: core.Endpoints{ @@ -20796,8 +20812,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "a", Port: 93, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "format=ip-sloppy", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=ip-sloppy"), + }, }, "Port missing number": { endpoints: core.Endpoints{ @@ -20807,8 +20824,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "a", Protocol: "TCP"}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "portNum", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].ports[0].port"), nil, "").WithOrigin("portNum"), + }, }, "Port missing protocol": { endpoints: core.Endpoints{ @@ -20818,7 +20836,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "a", Port: 93}}, }}, }, - errorType: "FieldValueRequired", + expectedErrs: field.ErrorList{ + field.Required(field.NewPath("subsets[0].ports[0].protocol"), ""), + }, }, "Address is loopback": { endpoints: core.Endpoints{ @@ -20828,8 +20848,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "p", Port: 93, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "format=non-special-ip", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"), + }, }, "Address is link-local": { endpoints: core.Endpoints{ @@ -20839,8 +20860,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "p", Port: 93, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "format=non-special-ip", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"), + }, }, "Address is link-local multicast": { endpoints: core.Endpoints{ @@ -20850,8 +20872,9 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "p", Port: 93, Protocol: "TCP"}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "format=non-special-ip", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].addresses[0].ip"), nil, "").WithOrigin("format=non-special-ip"), + }, }, "Invalid AppProtocol": { endpoints: core.Endpoints{ @@ -20861,16 +20884,18 @@ func TestValidateEndpointsCreate(t *testing.T) { Ports: []core.EndpointPort{{Name: "p", Port: 93, Protocol: "TCP", AppProtocol: utilpointer.String("lots-of[invalid]-{chars}")}}, }}, }, - errorType: "FieldValueInvalid", - errorOrigin: "format=qualified-name", + expectedErrs: field.ErrorList{ + field.Invalid(field.NewPath("subsets[0].ports[0].appProtocol"), nil, "").WithOrigin("format=qualified-name"), + }, }, } for k, v := range errorCases { t.Run(k, func(t *testing.T) { - if errs := ValidateEndpointsCreate(&v.endpoints); len(errs) == 0 || errs[0].Type != v.errorType || errs[0].Origin != v.errorOrigin { - t.Errorf("Expected error type %s with origin %q, got %#v", v.errorType, v.errorOrigin, errs[0]) - } + errs := ValidateEndpointsCreate(&v.endpoints) + // TODO: set .RequireOriginWhenInvalid() once metadata is done + matcher := fldtest.Match().ByType().ByField().ByOrigin() + fldtest.MatchErrors(t, v.expectedErrs, errs, matcher) }) } } @@ -22630,8 +22655,8 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { testCases := []struct { name string constraints []core.TopologySpreadConstraint - wantFieldErrors field.ErrorList opts PodValidationOptions + wantFieldErrors field.ErrorList }{{ name: "all required fields ok", constraints: []core.TopologySpreadConstraint{{ @@ -22640,19 +22665,23 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { WhenUnsatisfiable: core.DoNotSchedule, MinDomains: utilpointer.Int32(3), }}, - wantFieldErrors: field.ErrorList{}, + wantFieldErrors: nil, }, { name: "missing MaxSkew", constraints: []core.TopologySpreadConstraint{ {TopologyKey: "k8s.io/zone", WhenUnsatisfiable: core.DoNotSchedule}, }, - wantFieldErrors: []*field.Error{field.Invalid(fieldPathMaxSkew, int32(0), isNotPositiveErrorMsg)}, + wantFieldErrors: field.ErrorList{ + field.Invalid(fieldPathMaxSkew, nil, "").WithOrigin("minimum"), + }, }, { name: "negative MaxSkew", constraints: []core.TopologySpreadConstraint{ {MaxSkew: -1, TopologyKey: "k8s.io/zone", WhenUnsatisfiable: core.DoNotSchedule}, }, - wantFieldErrors: []*field.Error{field.Invalid(fieldPathMaxSkew, int32(-1), isNotPositiveErrorMsg)}, + wantFieldErrors: field.ErrorList{ + field.Invalid(fieldPathMaxSkew, nil, "").WithOrigin("minimum"), + }, }, { name: "can use MinDomains with ScheduleAnyway, when MinDomains = nil", constraints: []core.TopologySpreadConstraint{{ @@ -22661,7 +22690,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { WhenUnsatisfiable: core.ScheduleAnyway, MinDomains: nil, }}, - wantFieldErrors: field.ErrorList{}, + wantFieldErrors: nil, }, { name: "negative minDomains is invalid", constraints: []core.TopologySpreadConstraint{{ @@ -22670,7 +22699,9 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { WhenUnsatisfiable: core.DoNotSchedule, MinDomains: utilpointer.Int32(-1), }}, - wantFieldErrors: []*field.Error{field.Invalid(fieldPathMinDomains, utilpointer.Int32(-1), isNotPositiveErrorMsg)}, + wantFieldErrors: field.ErrorList{ + field.Invalid(fieldPathMinDomains, nil, "").WithOrigin("minimum"), + }, }, { name: "cannot use non-nil MinDomains with ScheduleAnyway", constraints: []core.TopologySpreadConstraint{{ @@ -22679,7 +22710,9 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { WhenUnsatisfiable: core.ScheduleAnyway, MinDomains: utilpointer.Int32(10), }}, - wantFieldErrors: []*field.Error{field.Invalid(fieldPathMinDomains, utilpointer.Int32(10), fmt.Sprintf("can only use minDomains if whenUnsatisfiable=%s, not %s", string(core.DoNotSchedule), string(core.ScheduleAnyway)))}, + wantFieldErrors: field.ErrorList{ + field.Invalid(fieldPathMinDomains, nil, "").WithOrigin("dependsOn"), + }, }, { name: "use negative MinDomains with ScheduleAnyway(invalid)", constraints: []core.TopologySpreadConstraint{{ @@ -22688,50 +22721,58 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { WhenUnsatisfiable: core.ScheduleAnyway, MinDomains: utilpointer.Int32(-1), }}, - wantFieldErrors: []*field.Error{ - field.Invalid(fieldPathMinDomains, utilpointer.Int32(-1), isNotPositiveErrorMsg), - field.Invalid(fieldPathMinDomains, utilpointer.Int32(-1), fmt.Sprintf("can only use minDomains if whenUnsatisfiable=%s, not %s", string(core.DoNotSchedule), string(core.ScheduleAnyway))), + wantFieldErrors: field.ErrorList{ + field.Invalid(fieldPathMinDomains, nil, "").WithOrigin("minimum"), + field.Invalid(fieldPathMinDomains, nil, "").WithOrigin("dependsOn"), }, }, { name: "missing TopologyKey", constraints: []core.TopologySpreadConstraint{ {MaxSkew: 1, WhenUnsatisfiable: core.DoNotSchedule}, }, - wantFieldErrors: []*field.Error{field.Required(fieldPathTopologyKey, "can not be empty")}, + wantFieldErrors: field.ErrorList{ + field.Required(fieldPathTopologyKey, ""), + }, }, { name: "missing scheduling mode", constraints: []core.TopologySpreadConstraint{ {MaxSkew: 1, TopologyKey: "k8s.io/zone"}, }, - wantFieldErrors: []*field.Error{field.NotSupported(fieldPathWhenUnsatisfiable, core.UnsatisfiableConstraintAction(""), sets.List(supportedScheduleActions))}, + wantFieldErrors: field.ErrorList{ + field.NotSupported[core.UnsatisfiableConstraintAction](fieldPathWhenUnsatisfiable, nil, nil), + }, }, { name: "unsupported scheduling mode", constraints: []core.TopologySpreadConstraint{ {MaxSkew: 1, TopologyKey: "k8s.io/zone", WhenUnsatisfiable: core.UnsatisfiableConstraintAction("N/A")}, }, - wantFieldErrors: []*field.Error{field.NotSupported(fieldPathWhenUnsatisfiable, core.UnsatisfiableConstraintAction("N/A"), sets.List(supportedScheduleActions))}, + wantFieldErrors: field.ErrorList{ + field.NotSupported[core.UnsatisfiableConstraintAction](fieldPathWhenUnsatisfiable, nil, nil), + }, }, { name: "multiple constraints ok with all required fields", constraints: []core.TopologySpreadConstraint{ {MaxSkew: 1, TopologyKey: "k8s.io/zone", WhenUnsatisfiable: core.DoNotSchedule}, {MaxSkew: 2, TopologyKey: "k8s.io/node", WhenUnsatisfiable: core.ScheduleAnyway}, }, - wantFieldErrors: field.ErrorList{}, + wantFieldErrors: nil, }, { name: "multiple constraints missing TopologyKey on partial ones", constraints: []core.TopologySpreadConstraint{ {MaxSkew: 1, WhenUnsatisfiable: core.ScheduleAnyway}, {MaxSkew: 2, TopologyKey: "k8s.io/zone", WhenUnsatisfiable: core.DoNotSchedule}, }, - wantFieldErrors: []*field.Error{field.Required(fieldPathTopologyKey, "can not be empty")}, + wantFieldErrors: field.ErrorList{ + field.Required(fieldPathTopologyKey, ""), + }, }, { name: "duplicate constraints", constraints: []core.TopologySpreadConstraint{ {MaxSkew: 1, TopologyKey: "k8s.io/zone", WhenUnsatisfiable: core.DoNotSchedule}, {MaxSkew: 2, TopologyKey: "k8s.io/zone", WhenUnsatisfiable: core.DoNotSchedule}, }, - wantFieldErrors: []*field.Error{ - field.Duplicate(fieldPathTopologyKeyAndWhenUnsatisfiable, fmt.Sprintf("{%v, %v}", "k8s.io/zone", core.DoNotSchedule)), + wantFieldErrors: field.ErrorList{ + field.Duplicate(fieldPathTopologyKeyAndWhenUnsatisfiable, nil), }, }, { name: "supported policy name set on NodeAffinityPolicy and NodeTaintsPolicy", @@ -22742,7 +22783,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { NodeAffinityPolicy: &honor, NodeTaintsPolicy: &ignore, }}, - wantFieldErrors: []*field.Error{}, + wantFieldErrors: nil, }, { name: "unsupported policy name set on NodeAffinityPolicy", constraints: []core.TopologySpreadConstraint{{ @@ -22752,8 +22793,8 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { NodeAffinityPolicy: &unknown, NodeTaintsPolicy: &ignore, }}, - wantFieldErrors: []*field.Error{ - field.NotSupported(nodeAffinityField, &unknown, sets.List(supportedPodTopologySpreadNodePolicies)), + wantFieldErrors: field.ErrorList{ + field.NotSupported[core.NodeInclusionPolicy](nodeAffinityField, nil, nil), }, }, { name: "unsupported policy name set on NodeTaintsPolicy", @@ -22764,8 +22805,8 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { NodeAffinityPolicy: &honor, NodeTaintsPolicy: &unknown, }}, - wantFieldErrors: []*field.Error{ - field.NotSupported(nodeTaintsField, &unknown, sets.List(supportedPodTopologySpreadNodePolicies)), + wantFieldErrors: field.ErrorList{ + field.NotSupported[core.NodeInclusionPolicy](nodeTaintsField, nil, nil), }, }, { name: "key in MatchLabelKeys isn't correctly defined", @@ -22776,7 +22817,9 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { WhenUnsatisfiable: core.DoNotSchedule, MatchLabelKeys: []string{"/simple"}, }}, - wantFieldErrors: field.ErrorList{field.Invalid(fieldPathMatchLabelKeys.Index(0), "/simple", "prefix part must be non-empty")}, + wantFieldErrors: field.ErrorList{ + field.Invalid(fieldPathMatchLabelKeys.Index(0), nil, "").WithOrigin("labelKey"), + }, }, { name: "key exists in both matchLabelKeys and labelSelector", constraints: []core.TopologySpreadConstraint{{ @@ -22794,7 +22837,9 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { }, }, }}, - wantFieldErrors: field.ErrorList{field.Invalid(fieldPathMatchLabelKeys.Index(0), "foo", "exists in both matchLabelKeys and labelSelector")}, + wantFieldErrors: field.ErrorList{ + field.Invalid(fieldPathMatchLabelKeys.Index(0), nil, "").WithOrigin("overlappingKeys"), + }, }, { name: "key in MatchLabelKeys is forbidden to be specified when labelSelector is not set", constraints: []core.TopologySpreadConstraint{{ @@ -22803,7 +22848,9 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { WhenUnsatisfiable: core.DoNotSchedule, MatchLabelKeys: []string{"foo"}, }}, - wantFieldErrors: field.ErrorList{field.Forbidden(fieldPathMatchLabelKeys, "must not be specified when labelSelector is not set")}, + wantFieldErrors: field.ErrorList{ + field.Forbidden(fieldPathMatchLabelKeys, ""), + }, }, { name: "invalid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is false", constraints: []core.TopologySpreadConstraint{{ @@ -22813,8 +22860,10 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { MinDomains: nil, LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "foo"}}, }}, - wantFieldErrors: []*field.Error{field.Invalid(labelSelectorField.Child("matchLabels"), "NoUppercaseOrSpecialCharsLike=Equals", "name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')")}, - opts: PodValidationOptions{AllowInvalidTopologySpreadConstraintLabelSelector: false}, + wantFieldErrors: field.ErrorList{ + field.Invalid(labelSelectorField.Child("matchLabels"), nil, "").WithOrigin("labelKey"), + }, + opts: PodValidationOptions{AllowInvalidTopologySpreadConstraintLabelSelector: false}, }, { name: "invalid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is true", constraints: []core.TopologySpreadConstraint{{ @@ -22824,7 +22873,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { MinDomains: nil, LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"NoUppercaseOrSpecialCharsLike=Equals": "foo"}}, }}, - wantFieldErrors: []*field.Error{}, + wantFieldErrors: nil, opts: PodValidationOptions{AllowInvalidTopologySpreadConstraintLabelSelector: true}, }, { name: "valid matchLabels set on labelSelector when AllowInvalidTopologySpreadConstraintLabelSelector is false", @@ -22835,17 +22884,15 @@ func TestValidateTopologySpreadConstraints(t *testing.T) { MinDomains: nil, LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "foo"}}, }}, - wantFieldErrors: []*field.Error{}, + wantFieldErrors: nil, opts: PodValidationOptions{AllowInvalidTopologySpreadConstraintLabelSelector: false}, - }, - } + }} for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { errs := validateTopologySpreadConstraints(tc.constraints, fieldPath, tc.opts) - if diff := cmp.Diff(tc.wantFieldErrors, errs); diff != "" { - t.Errorf("unexpected field errors (-want, +got):\n%s", diff) - } + matcher := fldtest.Match().ByType().ByField().ByOrigin().RequireOriginWhenInvalid() + fldtest.MatchErrors(t, tc.wantFieldErrors, errs, matcher) }) } } diff --git a/pkg/proxy/apis/config/validation/validation_test.go b/pkg/proxy/apis/config/validation/validation_test.go index cd964bcfba8..b80ab4354e4 100644 --- a/pkg/proxy/apis/config/validation/validation_test.go +++ b/pkg/proxy/apis/config/validation/validation_test.go @@ -21,6 +21,8 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -517,7 +519,10 @@ func TestValidateClientConnectionConfiguration(t *testing.T) { } { t.Run(name, func(t *testing.T) { errs := validateClientConnectionConfiguration(testCase.ccc, newPath) - assert.Equal(t, testCase.expectedErrs, errs, "did not get expected validation errors") + cmpOpts := []cmp.Option{cmpopts.IgnoreFields(field.Error{}, "Origin"), cmpopts.SortSlices(func(a, b *field.Error) bool { return a.Error() < b.Error() })} + if diff := cmp.Diff(testCase.expectedErrs, errs, cmpOpts...); diff != "" { + t.Errorf("did not get expected validation errors (-want,+got):\n%s", diff) + } }) } } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go index b1eb1bbfc44..95240b74d26 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go @@ -104,7 +104,7 @@ func ValidateLabelSelectorRequirement(sr metav1.LabelSelectorRequirement, opts L func ValidateLabelName(labelName string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for _, msg := range validation.IsQualifiedName(labelName) { - allErrs = append(allErrs, field.Invalid(fldPath, labelName, msg)) + allErrs = append(allErrs, field.Invalid(fldPath, labelName, msg).WithOrigin("labelKey")) } return allErrs } 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 c51c6832a35..4ada6bed1da 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 @@ -52,8 +52,8 @@ type Error struct { var _ error = &Error{} // Error implements the error interface. -func (v *Error) Error() string { - return fmt.Sprintf("%s: %s", v.Field, v.ErrorBody()) +func (e *Error) Error() string { + return fmt.Sprintf("%s: %s", e.Field, e.ErrorBody()) } type OmitValueType struct{} @@ -62,21 +62,21 @@ 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 { +func (e *Error) ErrorBody() string { var s string 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() + case e.Type == ErrorTypeRequired: + 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 := v.BadValue + value := e.BadValue valueType := reflect.TypeOf(value) if value == nil || valueType == nil { value = "null" @@ -90,30 +90,30 @@ func (v *Error) ErrorBody() string { switch t := value.(type) { case int64, int32, float64, float32, bool: // use simple printer for simple types - s = fmt.Sprintf("%s: %v", v.Type, value) + s = fmt.Sprintf("%s: %v", e.Type, value) case string: - s = fmt.Sprintf("%s: %q", v.Type, t) + 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", v.Type, t.String()) + 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", v.Type, value) + s = fmt.Sprintf("%s: %#v", e.Type, value) } } - if len(v.Detail) != 0 { - s += fmt.Sprintf(": %s", v.Detail) + if len(e.Detail) != 0 { + s += fmt.Sprintf(": %s", e.Detail) } return s } // WithOrigin adds origin information to the FieldError -func (v *Error) WithOrigin(o string) *Error { - v.Origin = o - return v +func (e *Error) WithOrigin(o string) *Error { + e.Origin = o + return e } // ErrorType is a machine readable value providing more detail about why diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/testing/match.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/testing/match.go new file mode 100644 index 00000000000..579cbda0c48 --- /dev/null +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/testing/match.go @@ -0,0 +1,203 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package testing + +import ( + "fmt" + "reflect" + "regexp" + "strings" + "testing" + + field "k8s.io/apimachinery/pkg/util/validation/field" +) + +// MatchErrors compares two ErrorLists with the specified matcher, and fails +// the test if they don't match. If a given "want" error matches multiple "got" +// errors, they will all be consumed. This might be OK (e.g. if there are +// multiple errors on the same field from the same origin) or it might be an +// insufficiently specific matcher, so these will be logged. +func MatchErrors(t *testing.T, want, got field.ErrorList, matcher *Matcher) { + t.Helper() + + remaining := got + for _, w := range want { + tmp := make(field.ErrorList, 0, len(remaining)) + n := 0 + for _, g := range remaining { + if matcher.Matches(w, g) { + n++ + } else { + tmp = append(tmp, g) + } + } + if n == 0 { + t.Errorf("expected an error matching:\n%s", matcher.Render(w)) + } else if n > 1 { + // This is not necessarily and error, but it's worth logging in + // case it's not what the test author intended. + t.Logf("multiple errors matched:\n%s", matcher.Render(w)) + } + remaining = tmp + } + if len(remaining) > 0 { + for _, e := range remaining { + t.Errorf("unmatched error:\n%s", Match().Exactly().Render(e)) + } + } +} + +// Match returns a new Matcher. +func Match() *Matcher { + return &Matcher{} +} + +// Matcher is a helper for comparing field.Error objects. +type Matcher struct { + matchType bool + matchField bool + matchValue bool + matchOrigin bool + matchDetail func(want, got string) bool + requireOriginWhenInvalid bool +} + +// Matches returns true if the two field.Error objects match according to the +// configured criteria. +func (m *Matcher) Matches(want, got *field.Error) bool { + if m.matchType && want.Type != got.Type { + return false + } + if m.matchField && want.Field != got.Field { + return false + } + if m.matchValue && !reflect.DeepEqual(want.BadValue, got.BadValue) { + return false + } + if m.matchOrigin { + if want.Origin != got.Origin { + return false + } + if m.requireOriginWhenInvalid && want.Type == field.ErrorTypeInvalid { + if want.Origin == "" || got.Origin == "" { + return false + } + } + } + if m.matchDetail != nil && !m.matchDetail(want.Detail, got.Detail) { + return false + } + return true +} + +// Render returns a string representation of the specified Error object, +// according to the criteria configured in the Matcher. +func (m *Matcher) Render(e *field.Error) string { + buf := strings.Builder{} + + comma := func() { + if buf.Len() > 0 { + buf.WriteString(", ") + } + } + + if m.matchType { + comma() + buf.WriteString(fmt.Sprintf("Type=%q", e.Type)) + } + if m.matchField { + comma() + buf.WriteString(fmt.Sprintf("Field=%q", e.Field)) + } + if m.matchValue { + comma() + buf.WriteString(fmt.Sprintf("Value=%v", e.BadValue)) + } + if m.matchOrigin || m.requireOriginWhenInvalid && e.Type == field.ErrorTypeInvalid { + comma() + buf.WriteString(fmt.Sprintf("Origin=%q", e.Origin)) + } + if m.matchDetail != nil { + comma() + buf.WriteString(fmt.Sprintf("Detail=%q", e.Detail)) + } + return "{" + buf.String() + "}" +} + +// Exactly configures the matcher to match all fields exactly. +func (m *Matcher) Exactly() *Matcher { + return m.ByType().ByField().ByValue().ByOrigin().ByDetailExact() +} + +// Exactly configures the matcher to match errors by type. +func (m *Matcher) ByType() *Matcher { + m.matchType = true + return m +} + +// ByField configures the matcher to match errors by field path. +func (m *Matcher) ByField() *Matcher { + m.matchField = true + return m +} + +// ByValue configures the matcher to match errors by the errant value. +func (m *Matcher) ByValue() *Matcher { + m.matchValue = true + return m +} + +// ByOrigin configures the matcher to match errors by the origin. +func (m *Matcher) ByOrigin() *Matcher { + m.matchOrigin = true + return m +} + +// RequireOriginWhenInvalid configures the matcher to require the Origin field +// to be set when the Type is Invalid and the matcher is matching by Origin. +func (m *Matcher) RequireOriginWhenInvalid() *Matcher { + m.requireOriginWhenInvalid = true + return m +} + +// ByDetailExact configures the matcher to match errors by the exact detail +// string. +func (m *Matcher) ByDetailExact() *Matcher { + m.matchDetail = func(want, got string) bool { + return got == want + } + return m +} + +// ByDetailSubstring configures the matcher to match errors by a substring of +// the detail string. +func (m *Matcher) ByDetailSubstring() *Matcher { + m.matchDetail = func(want, got string) bool { + return strings.Contains(got, want) + } + return m +} + +// ByDetailRegexp configures the matcher to match errors by a regular +// expression of the detail string, where the "want" string is assumed to be a +// valid regular expression. +func (m *Matcher) ByDetailRegexp() *Matcher { + m.matchDetail = func(want, got string) bool { + return regexp.MustCompile(want).MatchString(got) + } + return m +}