From 02f7dc55d162fcde7377b1cb0c5e2f9e8974548e Mon Sep 17 00:00:00 2001 From: yongruilin Date: Fri, 21 Feb 2025 20:20:47 +0000 Subject: [PATCH 1/4] feat: Add Origin field to Error and related methods This change introducing a new field in Error. It would be used in testing to compare the expected errors without matching the detail strings. Co-authored-by: Tim Hockin --- .../pkg/util/validation/field/errors.go | 48 +++++++++++++++---- .../pkg/util/validation/field/errors_test.go | 38 +++++++++++++++ 2 files changed, 76 insertions(+), 10 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 f1634bc0df8..c51c6832a35 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 @@ -33,6 +33,20 @@ type Error struct { Field string BadValue interface{} Detail string + + // Origin uniquely identifies where this error was generated from. It is used in testing to + // compare expected errors against actual errors without relying on exact detail string matching. + // This allows tests to verify the correct validation logic triggered the error + // regardless of how the error message might be formatted or localized. + // + // The value should be either: + // - A simple camelCase identifier (e.g., "maximum", "maxItems") + // - A structured format using "format=" for validation errors related to specific formats + // (e.g., "format=dns-label", "format=qualified-name") + // + // Origin should be set in the most deeply nested validation function that + // can still identify the unique source of the error. + Origin string } var _ error = &Error{} @@ -96,6 +110,12 @@ func (v *Error) ErrorBody() string { return s } +// WithOrigin adds origin information to the FieldError +func (v *Error) WithOrigin(o string) *Error { + v.Origin = o + return v +} + // ErrorType is a machine readable value providing more detail about why // a field is invalid. These values are expected to match 1-1 with // CauseType in api/types.go. @@ -169,32 +189,32 @@ func (t ErrorType) String() string { // TypeInvalid returns a *Error indicating "type is invalid" func TypeInvalid(field *Path, value interface{}, detail string) *Error { - return &Error{ErrorTypeTypeInvalid, field.String(), value, detail} + return &Error{ErrorTypeTypeInvalid, field.String(), value, detail, ""} } // NotFound returns a *Error indicating "value not found". This is // used to report failure to find a requested value (e.g. looking up an ID). func NotFound(field *Path, value interface{}) *Error { - return &Error{ErrorTypeNotFound, field.String(), value, ""} + return &Error{ErrorTypeNotFound, field.String(), value, "", ""} } // Required returns a *Error indicating "value required". This is used // to report required values that are not provided (e.g. empty strings, null // values, or empty arrays). func Required(field *Path, detail string) *Error { - return &Error{ErrorTypeRequired, field.String(), "", detail} + return &Error{ErrorTypeRequired, field.String(), "", detail, ""} } // Duplicate returns a *Error indicating "duplicate value". This is // used to report collisions of values that must be unique (e.g. names or IDs). func Duplicate(field *Path, value interface{}) *Error { - return &Error{ErrorTypeDuplicate, field.String(), value, ""} + return &Error{ErrorTypeDuplicate, field.String(), value, "", ""} } // Invalid returns a *Error indicating "invalid value". This is used // to report malformed values (e.g. failed regex match, too long, out of bounds). func Invalid(field *Path, value interface{}, detail string) *Error { - return &Error{ErrorTypeInvalid, field.String(), value, detail} + return &Error{ErrorTypeInvalid, field.String(), value, detail, ""} } // NotSupported returns a *Error indicating "unsupported value". @@ -209,7 +229,7 @@ func NotSupported[T ~string](field *Path, value interface{}, validValues []T) *E } detail = "supported values: " + strings.Join(quotedValues, ", ") } - return &Error{ErrorTypeNotSupported, field.String(), value, detail} + return &Error{ErrorTypeNotSupported, field.String(), value, detail, ""} } // Forbidden returns a *Error indicating "forbidden". This is used to @@ -217,7 +237,7 @@ func NotSupported[T ~string](field *Path, value interface{}, validValues []T) *E // some conditions, but which are not permitted by current conditions (e.g. // security policy). func Forbidden(field *Path, detail string) *Error { - return &Error{ErrorTypeForbidden, field.String(), "", detail} + return &Error{ErrorTypeForbidden, field.String(), "", detail, ""} } // TooLong returns a *Error indicating "too long". This is used to report that @@ -231,7 +251,7 @@ func TooLong(field *Path, value interface{}, maxLength int) *Error { } else { msg = "value is too long" } - return &Error{ErrorTypeTooLong, field.String(), "", msg} + return &Error{ErrorTypeTooLong, field.String(), "", msg, ""} } // TooLongMaxLength returns a *Error indicating "too long". @@ -259,14 +279,14 @@ func TooMany(field *Path, actualQuantity, maxQuantity int) *Error { actual = omitValue } - return &Error{ErrorTypeTooMany, field.String(), actual, msg} + return &Error{ErrorTypeTooMany, field.String(), actual, msg, ""} } // InternalError returns a *Error indicating "internal error". This is used // to signal that an error was found that was not directly related to user // input. The err argument must be non-nil. func InternalError(field *Path, err error) *Error { - return &Error{ErrorTypeInternal, field.String(), nil, err.Error()} + return &Error{ErrorTypeInternal, field.String(), nil, err.Error(), ""} } // ErrorList holds a set of Errors. It is plausible that we might one day have @@ -285,6 +305,14 @@ func NewErrorTypeMatcher(t ErrorType) utilerrors.Matcher { } } +// WithOrigin sets the origin for all errors in the list and returns the updated list. +func (list ErrorList) WithOrigin(origin string) ErrorList { + for _, err := range list { + err.Origin = origin + } + return list +} + // ToAggregate converts the ErrorList into an errors.Aggregate. func (list ErrorList) ToAggregate() utilerrors.Aggregate { if len(list) == 0 { 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 a2f9763b8ef..7c4c650edec 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 @@ -173,3 +173,41 @@ func TestNotSupported(t *testing.T) { t.Errorf("Expected: %s\n, but got: %s\n", expected, notSupported.ErrorBody()) } } + +func TestErrorOrigin(t *testing.T) { + err := Invalid(NewPath("field"), "value", "detail") + + // Test WithOrigin + newErr := err.WithOrigin("origin1") + if newErr.Origin != "origin1" { + t.Errorf("Expected Origin to be 'origin1', got '%s'", newErr.Origin) + } + if err.Origin != "origin1" { + t.Errorf("Expected Origin to be 'origin1', got '%s'", err.Origin) + } +} + +func TestErrorListOrigin(t *testing.T) { + // Create an ErrorList with multiple errors + list := ErrorList{ + Invalid(NewPath("field1"), "value1", "detail1"), + Invalid(NewPath("field2"), "value2", "detail2"), + Required(NewPath("field3"), "detail3"), + } + + // Test WithOrigin + newList := list.WithOrigin("origin1") + // Check that WithOrigin returns the modified list + for i, err := range newList { + if err.Origin != "origin1" { + t.Errorf("Error %d: Expected Origin to be 'origin2', got '%s'", i, err.Origin) + } + } + + // Check that the original list was also modified (WithOrigin modifies and returns the same list) + for i, err := range list { + if err.Origin != "origin1" { + t.Errorf("Error %d: Expected original list Origin to be 'origin2', got '%s'", i, err.Origin) + } + } +} From a4885091974a005f388e1d0cb6b18ebb829af847 Mon Sep 17 00:00:00 2001 From: yongruilin Date: Fri, 21 Feb 2025 22:30:23 +0000 Subject: [PATCH 2/4] test: Improve error comparison in resource validation tests Replace manual error logging with cmp.Diff for more precise error comparisons, using cmpopts to ignore Origin field and support UniqueString comparison. --- .../validation/validation_common_test.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/pkg/apis/resource/validation/validation_common_test.go b/pkg/apis/resource/validation/validation_common_test.go index 20f1b71abd0..45e89515865 100644 --- a/pkg/apis/resource/validation/validation_common_test.go +++ b/pkg/apis/resource/validation/validation_common_test.go @@ -19,9 +19,12 @@ package validation import ( "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/dynamic-resource-allocation/api" ) // assertFailures compares the expected against the actual errors. @@ -31,22 +34,13 @@ import ( // is informative. func assertFailures(tb testing.TB, want, got field.ErrorList) bool { tb.Helper() - if !assert.Equal(tb, want, got) { - logFailures(tb, "Wanted failures", want) - logFailures(tb, "Got failures", got) + if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(field.Error{}, "Origin"), cmp.AllowUnexported(api.UniqueString{})); diff != "" { + tb.Errorf("unexpected field errors (-want, +got):\n%s", diff) return false } return true } -func logFailures(tb testing.TB, header string, errs field.ErrorList) { - tb.Helper() - tb.Logf("%s:\n", header) - for _, err := range errs { - tb.Logf("- %s\n", err) - } -} - func TestTruncateIfTooLong(t *testing.T) { for name, tc := range map[string]struct { str string From 07477c656e7f3aa8c9d44e769bd2dfbdcc374b85 Mon Sep 17 00:00:00 2001 From: yongruilin Date: Fri, 21 Feb 2025 22:34:00 +0000 Subject: [PATCH 3/4] test: convert ValidateEndpointsCreate to use error Origin field in test Update ValidateEndpointsCreate validation tests to use the new Origin field for more precise error comparisons. It leverage the Origin field instead of detailed error messages, improving test robustness and readability. Co-authored-by: Tim Hockin --- pkg/apis/core/validation/validation.go | 18 +++++----- pkg/apis/core/validation/validation_test.go | 36 +++++++++---------- .../pkg/util/validation/validation.go | 2 +- 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 8a61d65ddd1..3f35be6e07d 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -133,7 +133,7 @@ func ValidateAnnotations(annotations map[string]string, fldPath *field.Path) fie func ValidateDNS1123Label(value string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for _, msg := range validation.IsDNS1123Label(value) { - allErrs = append(allErrs, field.Invalid(fldPath, value, msg)) + allErrs = append(allErrs, field.Invalid(fldPath, value, msg).WithOrigin("format=dns-label")) } return allErrs } @@ -142,7 +142,7 @@ func ValidateDNS1123Label(value string, fldPath *field.Path) field.ErrorList { func ValidateQualifiedName(value string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} for _, msg := range validation.IsQualifiedName(value) { - allErrs = append(allErrs, field.Invalid(fldPath, value, msg)) + allErrs = append(allErrs, field.Invalid(fldPath, value, msg).WithOrigin("format=qualified-name")) } return allErrs } @@ -7466,7 +7466,7 @@ func validateEndpointAddress(address *core.EndpointAddress, fldPath *field.Path) // During endpoint update, verify that NodeName is a DNS subdomain and transition rules allow the update if address.NodeName != nil { for _, msg := range ValidateNodeName(*address.NodeName, false) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("nodeName"), *address.NodeName, msg)) + allErrs = append(allErrs, field.Invalid(fldPath.Child("nodeName"), *address.NodeName, msg).WithOrigin("format=dns-label")) } } allErrs = append(allErrs, ValidateNonSpecialIP(address.IP, fldPath.Child("ip"))...) @@ -7485,20 +7485,20 @@ func ValidateNonSpecialIP(ipAddress string, fldPath *field.Path) field.ErrorList allErrs := field.ErrorList{} ip := netutils.ParseIPSloppy(ipAddress) if ip == nil { - allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "must be a valid IP address")) + allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "must be a valid IP address").WithOrigin("format=ip-sloppy")) return allErrs } if ip.IsUnspecified() { - allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, fmt.Sprintf("may not be unspecified (%v)", ipAddress))) + allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, fmt.Sprintf("may not be unspecified (%v)", ipAddress)).WithOrigin("format=non-special-ip")) } if ip.IsLoopback() { - allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the loopback range (127.0.0.0/8, ::1/128)")) + allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the loopback range (127.0.0.0/8, ::1/128)").WithOrigin("format=non-special-ip")) } if ip.IsLinkLocalUnicast() { - allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the link-local range (169.254.0.0/16, fe80::/10)")) + allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the link-local range (169.254.0.0/16, fe80::/10)").WithOrigin("format=non-special-ip")) } if ip.IsLinkLocalMulticast() { - allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the link-local multicast range (224.0.0.0/24, ff02::/10)")) + allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the link-local multicast range (224.0.0.0/24, ff02::/10)").WithOrigin("format=non-special-ip")) } return allErrs } @@ -7511,7 +7511,7 @@ func validateEndpointPort(port *core.EndpointPort, requireName bool, fldPath *fi allErrs = append(allErrs, ValidateDNS1123Label(port.Name, fldPath.Child("name"))...) } for _, msg := range validation.IsValidPortNum(int(port.Port)) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("port"), port.Port, msg)) + allErrs = append(allErrs, field.Invalid(fldPath.Child("port"), port.Port, msg).WithOrigin("portNum")) } if len(port.Protocol) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("protocol"), "")) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 992ab9de564..4c6e0f64425 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -9183,7 +9183,7 @@ func TestValidateContainers(t *testing.T) { t.Fatal("expected error but received none") } - if diff := cmp.Diff(tc.expectedErrors, errs, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail")); diff != "" { + if diff := cmp.Diff(tc.expectedErrors, errs, cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail", "Origin")); diff != "" { t.Errorf("unexpected diff in errors (-want, +got):\n%s", diff) t.Errorf("INFO: all errors:\n%s", prettyErrorList(errs)) } @@ -20674,7 +20674,7 @@ func TestValidateEndpointsCreate(t *testing.T) { errorCases := map[string]struct { endpoints core.Endpoints errorType field.ErrorType - errorDetail string + errorOrigin string }{ "missing namespace": { endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "mysvc"}}, @@ -20685,14 +20685,12 @@ func TestValidateEndpointsCreate(t *testing.T) { errorType: "FieldValueRequired", }, "invalid namespace": { - endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "no@#invalid.;chars\"allowed"}}, - errorType: "FieldValueInvalid", - errorDetail: dnsLabelErrMsg, + endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "no@#invalid.;chars\"allowed"}}, + errorType: "FieldValueInvalid", }, "invalid name": { - endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "-_Invliad^&Characters", Namespace: "namespace"}}, - errorType: "FieldValueInvalid", - errorDetail: dnsSubdomainLabelErrMsg, + endpoints: core.Endpoints{ObjectMeta: metav1.ObjectMeta{Name: "-_Invliad^&Characters", Namespace: "namespace"}}, + errorType: "FieldValueInvalid", }, "empty addresses": { endpoints: core.Endpoints{ @@ -20712,7 +20710,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "must be a valid IP address", + errorOrigin: "format=ip-sloppy", }, "Multiple ports, one without name": { endpoints: core.Endpoints{ @@ -20733,7 +20731,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "between", + errorOrigin: "portNum", }, "Invalid protocol": { endpoints: core.Endpoints{ @@ -20754,7 +20752,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "must be a valid IP address", + errorOrigin: "format=ip-sloppy", }, "Port missing number": { endpoints: core.Endpoints{ @@ -20765,7 +20763,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "between", + errorOrigin: "portNum", }, "Port missing protocol": { endpoints: core.Endpoints{ @@ -20786,7 +20784,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "loopback", + errorOrigin: "format=non-special-ip", }, "Address is link-local": { endpoints: core.Endpoints{ @@ -20797,7 +20795,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "link-local", + errorOrigin: "format=non-special-ip", }, "Address is link-local multicast": { endpoints: core.Endpoints{ @@ -20808,7 +20806,7 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "link-local multicast", + errorOrigin: "format=non-special-ip", }, "Invalid AppProtocol": { endpoints: core.Endpoints{ @@ -20819,14 +20817,14 @@ func TestValidateEndpointsCreate(t *testing.T) { }}, }, errorType: "FieldValueInvalid", - errorDetail: "name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character", + errorOrigin: "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 || !strings.Contains(errs[0].Detail, v.errorDetail) { - t.Errorf("Expected error type %s with detail %q, got %v", v.errorType, v.errorDetail, errs) + 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]) } }) } @@ -21190,7 +21188,7 @@ func TestValidateSchedulingGates(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { errs := validateSchedulingGates(tt.schedulingGates, fieldPath) - if diff := cmp.Diff(tt.wantFieldErrors, errs); diff != "" { + if diff := cmp.Diff(tt.wantFieldErrors, errs, cmpopts.IgnoreFields(field.Error{}, "Detail", "Origin")); diff != "" { t.Errorf("unexpected field errors (-want, +got):\n%s", diff) } }) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go index 9bc393cf586..feb3d259b8a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go @@ -373,7 +373,7 @@ func IsValidPortName(port string) []string { func IsValidIP(fldPath *field.Path, value string) field.ErrorList { var allErrors field.ErrorList if netutils.ParseIPSloppy(value) == nil { - allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IP address, (e.g. 10.9.8.7 or 2001:db8::ffff)")) + allErrors = append(allErrors, field.Invalid(fldPath, value, "must be a valid IP address, (e.g. 10.9.8.7 or 2001:db8::ffff)").WithOrigin("format=ip-sloppy")) } return allErrors } From c7cf852086c4256d33549d6163708e0f70719ae6 Mon Sep 17 00:00:00 2001 From: yongruilin Date: Fri, 21 Feb 2025 23:37:34 +0000 Subject: [PATCH 4/4] test: Add Origin field support to ReplicationController spec.Replicas validation test --- pkg/apis/core/validation/validation.go | 2 +- pkg/apis/core/validation/validation_test.go | 215 ++++++++++++-------- 2 files changed, 131 insertions(+), 86 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 3f35be6e07d..875e2702954 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -6266,7 +6266,7 @@ func ValidateReplicationControllerSpec(spec, oldSpec *core.ReplicationController allErrs := field.ErrorList{} allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.MinReadySeconds), fldPath.Child("minReadySeconds"))...) allErrs = append(allErrs, ValidateNonEmptySelector(spec.Selector, fldPath.Child("selector"))...) - allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas"))...) + allErrs = append(allErrs, ValidateNonnegativeField(int64(spec.Replicas), fldPath.Child("replicas")).WithOrigin("minimum")...) allErrs = append(allErrs, ValidatePodTemplateSpecForRC(spec.Template, spec.Selector, spec.Replicas, fldPath.Child("template"), opts)...) return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 4c6e0f64425..f269cbaf96b 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -16791,144 +16791,179 @@ func TestValidateReplicationController(t *testing.T) { } } - errorCases := map[string]core.ReplicationController{ + errorCases := map[string]struct { + rc core.ReplicationController + expectedOrigin []string + }{ "zero-length ID": { - ObjectMeta: metav1.ObjectMeta{Name: "", Namespace: metav1.NamespaceDefault}, - Spec: core.ReplicationControllerSpec{ - Selector: validSelector, - Template: &validPodTemplate.Template, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "", Namespace: metav1.NamespaceDefault}, + Spec: core.ReplicationControllerSpec{ + Selector: validSelector, + Template: &validPodTemplate.Template, + }, }, }, "missing-namespace": { - ObjectMeta: metav1.ObjectMeta{Name: "abc-123"}, - Spec: core.ReplicationControllerSpec{ - Selector: validSelector, - Template: &validPodTemplate.Template, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc-123"}, + Spec: core.ReplicationControllerSpec{ + Selector: validSelector, + Template: &validPodTemplate.Template, + }, }, }, "empty selector": { - ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, - Spec: core.ReplicationControllerSpec{ - Template: &validPodTemplate.Template, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: core.ReplicationControllerSpec{ + Template: &validPodTemplate.Template, + }, }, }, "selector_doesnt_match": { - ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, - Spec: core.ReplicationControllerSpec{ - Selector: map[string]string{"foo": "bar"}, - Template: &validPodTemplate.Template, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: core.ReplicationControllerSpec{ + Selector: map[string]string{"foo": "bar"}, + Template: &validPodTemplate.Template, + }, }, }, "invalid manifest": { - ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, - Spec: core.ReplicationControllerSpec{ - Selector: validSelector, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: core.ReplicationControllerSpec{ + Selector: validSelector, + }, }, }, "read-write persistent disk with > 1 pod": { - ObjectMeta: metav1.ObjectMeta{Name: "abc"}, - Spec: core.ReplicationControllerSpec{ - Replicas: 2, - Selector: validSelector, - Template: &readWriteVolumePodTemplate.Template, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc"}, + Spec: core.ReplicationControllerSpec{ + Replicas: 2, + Selector: validSelector, + Template: &readWriteVolumePodTemplate.Template, + }, }, }, "negative_replicas": { - ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, - Spec: core.ReplicationControllerSpec{ - Replicas: -1, - Selector: validSelector, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc", Namespace: metav1.NamespaceDefault}, + Spec: core.ReplicationControllerSpec{ + Replicas: -1, + Selector: validSelector, + }, + }, + expectedOrigin: []string{ + "minimum", }, }, "invalid_label": { - ObjectMeta: metav1.ObjectMeta{ - Name: "abc-123", - Namespace: metav1.NamespaceDefault, - Labels: map[string]string{ - "NoUppercaseOrSpecialCharsLike=Equals": "bar", + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc-123", + Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + "NoUppercaseOrSpecialCharsLike=Equals": "bar", + }, + }, + Spec: core.ReplicationControllerSpec{ + Selector: validSelector, + Template: &validPodTemplate.Template, }, - }, - Spec: core.ReplicationControllerSpec{ - Selector: validSelector, - Template: &validPodTemplate.Template, }, }, "invalid_label 2": { - ObjectMeta: metav1.ObjectMeta{ - Name: "abc-123", - Namespace: metav1.NamespaceDefault, - Labels: map[string]string{ - "NoUppercaseOrSpecialCharsLike=Equals": "bar", + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc-123", + Namespace: metav1.NamespaceDefault, + Labels: map[string]string{ + "NoUppercaseOrSpecialCharsLike=Equals": "bar", + }, + }, + Spec: core.ReplicationControllerSpec{ + Template: &invalidPodTemplate.Template, }, - }, - Spec: core.ReplicationControllerSpec{ - Template: &invalidPodTemplate.Template, }, }, "invalid_annotation": { - ObjectMeta: metav1.ObjectMeta{ - Name: "abc-123", - Namespace: metav1.NamespaceDefault, - Annotations: map[string]string{ - "NoUppercaseOrSpecialCharsLike=Equals": "bar", + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc-123", + Namespace: metav1.NamespaceDefault, + Annotations: map[string]string{ + "NoUppercaseOrSpecialCharsLike=Equals": "bar", + }, + }, + Spec: core.ReplicationControllerSpec{ + Selector: validSelector, + Template: &validPodTemplate.Template, }, - }, - Spec: core.ReplicationControllerSpec{ - Selector: validSelector, - Template: &validPodTemplate.Template, }, }, "invalid restart policy 1": { - ObjectMeta: metav1.ObjectMeta{ - Name: "abc-123", - Namespace: metav1.NamespaceDefault, - }, - Spec: core.ReplicationControllerSpec{ - Selector: validSelector, - Template: &core.PodTemplateSpec{ - Spec: podtest.MakePodSpec(podtest.SetRestartPolicy(core.RestartPolicyOnFailure)), - ObjectMeta: metav1.ObjectMeta{ - Labels: validSelector, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc-123", + Namespace: metav1.NamespaceDefault, + }, + Spec: core.ReplicationControllerSpec{ + Selector: validSelector, + Template: &core.PodTemplateSpec{ + Spec: podtest.MakePodSpec(podtest.SetRestartPolicy(core.RestartPolicyOnFailure)), + ObjectMeta: metav1.ObjectMeta{ + Labels: validSelector, + }, }, }, }, }, "invalid restart policy 2": { - ObjectMeta: metav1.ObjectMeta{ - Name: "abc-123", - Namespace: metav1.NamespaceDefault, - }, - Spec: core.ReplicationControllerSpec{ - Selector: validSelector, - Template: &core.PodTemplateSpec{ - Spec: podtest.MakePodSpec(podtest.SetRestartPolicy(core.RestartPolicyNever)), - ObjectMeta: metav1.ObjectMeta{ - Labels: validSelector, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "abc-123", + Namespace: metav1.NamespaceDefault, + }, + Spec: core.ReplicationControllerSpec{ + Selector: validSelector, + Template: &core.PodTemplateSpec{ + Spec: podtest.MakePodSpec(podtest.SetRestartPolicy(core.RestartPolicyNever)), + ObjectMeta: metav1.ObjectMeta{ + Labels: validSelector, + }, }, }, }, }, "template may not contain ephemeral containers": { - ObjectMeta: metav1.ObjectMeta{Name: "abc-123", Namespace: metav1.NamespaceDefault}, - Spec: core.ReplicationControllerSpec{ - Replicas: 1, - Selector: validSelector, - Template: &core.PodTemplateSpec{ - ObjectMeta: metav1.ObjectMeta{ - Labels: validSelector, + rc: core.ReplicationController{ + ObjectMeta: metav1.ObjectMeta{Name: "abc-123", Namespace: metav1.NamespaceDefault}, + Spec: core.ReplicationControllerSpec{ + Replicas: 1, + Selector: validSelector, + Template: &core.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: validSelector, + }, + Spec: podtest.MakePodSpec( + podtest.SetEphemeralContainers(core.EphemeralContainer{EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}), + ), }, - Spec: podtest.MakePodSpec( - podtest.SetEphemeralContainers(core.EphemeralContainer{EphemeralContainerCommon: core.EphemeralContainerCommon{Name: "debug", Image: "image", ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}}), - ), }, }, }, } for k, v := range errorCases { - errs := ValidateReplicationController(&v, PodValidationOptions{}) + errs := ValidateReplicationController(&v.rc, PodValidationOptions{}) if len(errs) == 0 { t.Errorf("expected failure for %s", k) } + + expectedOrigins := sets.NewString(v.expectedOrigin...) + for i := range errs { field := errs[i].Field if !strings.HasPrefix(field, "spec.template.") && @@ -16944,6 +16979,16 @@ func TestValidateReplicationController(t *testing.T) { field != "status.replicas" { t.Errorf("%s: missing prefix for: %v", k, errs[i]) } + + if len(v.expectedOrigin) > 0 && errs[i].Origin != "" { + if !expectedOrigins.Has(errs[i].Origin) { + t.Errorf("%s: unexpected origin for: %v, expected one of %v", k, errs[i].Origin, v.expectedOrigin) + } + expectedOrigins.Delete(errs[i].Origin) + } + } + if len(expectedOrigins) > 0 { + t.Errorf("%s: missing errors with origin: %v", k, expectedOrigins.List()) } } }