Merge pull request #17246 from thockin/airplane_validation_pt2

Auto commit by PR queue bot

pkg/api/errors/errors_test.go

@@ -125,7 +125,7 @@ func TestNewInvalid(t *testing.T) {
 			},
 		},
 		{
-			validation.NewFieldValueNotSupported("field[0].name", "bar", nil),
+			validation.NewFieldNotSupported("field[0].name", "bar", nil),
 			&unversioned.StatusDetails{
 				Kind: "kind",
 				Name: "name",

pkg/api/validation/validation.go

@@ -789,7 +789,7 @@ func validatePorts(ports []api.ContainerPort) validation.ErrorList {
 		if len(port.Protocol) == 0 {
 			pErrs = append(pErrs, validation.NewFieldRequired("protocol"))
 		} else if !supportedPortProtocols.Has(string(port.Protocol)) {
-			pErrs = append(pErrs, validation.NewFieldValueNotSupported("protocol", port.Protocol, supportedPortProtocols.List()))
+			pErrs = append(pErrs, validation.NewFieldNotSupported("protocol", port.Protocol, supportedPortProtocols.List()))
 		}
 		allErrs = append(allErrs, pErrs.PrefixIndex(i)...)
 	}
@@ -848,7 +848,7 @@ func validateObjectFieldSelector(fs *api.ObjectFieldSelector, expressions *sets.
 		if err != nil {
 			allErrs = append(allErrs, validation.NewFieldInvalid("fieldPath", fs.FieldPath, "error converting fieldPath"))
 		} else if !expressions.Has(internalFieldPath) {
-			allErrs = append(allErrs, validation.NewFieldValueNotSupported("fieldPath", internalFieldPath, expressions.List()))
+			allErrs = append(allErrs, validation.NewFieldNotSupported("fieldPath", internalFieldPath, expressions.List()))
 		}
 	}
 
@@ -996,7 +996,7 @@ func validatePullPolicy(ctr *api.Container) validation.ErrorList {
 		allErrors = append(allErrors, validation.NewFieldRequired(""))
 	default:
 		validValues := []string{string(api.PullAlways), string(api.PullIfNotPresent), string(api.PullNever)}
-		allErrors = append(allErrors, validation.NewFieldValueNotSupported("", ctr.ImagePullPolicy, validValues))
+		allErrors = append(allErrors, validation.NewFieldNotSupported("", ctr.ImagePullPolicy, validValues))
 	}
 
 	return allErrors
@@ -1057,7 +1057,7 @@ func validateRestartPolicy(restartPolicy *api.RestartPolicy) validation.ErrorLis
 		allErrors = append(allErrors, validation.NewFieldRequired(""))
 	default:
 		validValues := []string{string(api.RestartPolicyAlways), string(api.RestartPolicyOnFailure), string(api.RestartPolicyNever)}
-		allErrors = append(allErrors, validation.NewFieldValueNotSupported("", *restartPolicy, validValues))
+		allErrors = append(allErrors, validation.NewFieldNotSupported("", *restartPolicy, validValues))
 	}
 
 	return allErrors
@@ -1072,7 +1072,7 @@ func validateDNSPolicy(dnsPolicy *api.DNSPolicy) validation.ErrorList {
 		allErrors = append(allErrors, validation.NewFieldRequired(""))
 	default:
 		validValues := []string{string(api.DNSClusterFirst), string(api.DNSDefault)}
-		allErrors = append(allErrors, validation.NewFieldValueNotSupported("", dnsPolicy, validValues))
+		allErrors = append(allErrors, validation.NewFieldNotSupported("", dnsPolicy, validValues))
 	}
 	return allErrors
 }
@@ -1250,7 +1250,7 @@ func ValidateService(service *api.Service) validation.ErrorList {
 	if service.Spec.SessionAffinity == "" {
 		allErrs = append(allErrs, validation.NewFieldRequired("spec.sessionAffinity"))
 	} else if !supportedSessionAffinityType.Has(string(service.Spec.SessionAffinity)) {
-		allErrs = append(allErrs, validation.NewFieldValueNotSupported("spec.sessionAffinity", service.Spec.SessionAffinity, supportedSessionAffinityType.List()))
+		allErrs = append(allErrs, validation.NewFieldNotSupported("spec.sessionAffinity", service.Spec.SessionAffinity, supportedSessionAffinityType.List()))
 	}
 
 	if api.IsServiceIPSet(service) {
@@ -1269,7 +1269,7 @@ func ValidateService(service *api.Service) validation.ErrorList {
 	if service.Spec.Type == "" {
 		allErrs = append(allErrs, validation.NewFieldRequired("spec.type"))
 	} else if !supportedServiceType.Has(string(service.Spec.Type)) {
-		allErrs = append(allErrs, validation.NewFieldValueNotSupported("spec.type", service.Spec.Type, supportedServiceType.List()))
+		allErrs = append(allErrs, validation.NewFieldNotSupported("spec.type", service.Spec.Type, supportedServiceType.List()))
 	}
 
 	if service.Spec.Type == api.ServiceTypeLoadBalancer {
@@ -1330,7 +1330,7 @@ func validateServicePort(sp *api.ServicePort, requireName bool, allNames *sets.S
 	if len(sp.Protocol) == 0 {
 		allErrs = append(allErrs, validation.NewFieldRequired("protocol"))
 	} else if !supportedPortProtocols.Has(string(sp.Protocol)) {
-		allErrs = append(allErrs, validation.NewFieldValueNotSupported("protocol", sp.Protocol, supportedPortProtocols.List()))
+		allErrs = append(allErrs, validation.NewFieldNotSupported("protocol", sp.Protocol, supportedPortProtocols.List()))
 	}
 
 	if sp.TargetPort.Type == intstr.Int && !validation.IsValidPortNum(sp.TargetPort.IntVal) {
@@ -1411,7 +1411,7 @@ func ValidatePodTemplateSpecForRC(template *api.PodTemplateSpec, selectorMap map
 		}
 		// RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec().
 		if template.Spec.RestartPolicy != api.RestartPolicyAlways {
-			allErrs = append(allErrs, validation.NewFieldValueNotSupported(fieldName+".spec.restartPolicy", template.Spec.RestartPolicy, []string{string(api.RestartPolicyAlways)}))
+			allErrs = append(allErrs, validation.NewFieldNotSupported(fieldName+".spec.restartPolicy", template.Spec.RestartPolicy, []string{string(api.RestartPolicyAlways)}))
 		}
 	}
 	return allErrs
@@ -1974,7 +1974,7 @@ func validateEndpointPort(port *api.EndpointPort, requireName bool) validation.E
 	if len(port.Protocol) == 0 {
 		allErrs = append(allErrs, validation.NewFieldRequired("protocol"))
 	} else if !supportedPortProtocols.Has(string(port.Protocol)) {
-		allErrs = append(allErrs, validation.NewFieldValueNotSupported("protocol", port.Protocol, supportedPortProtocols.List()))
+		allErrs = append(allErrs, validation.NewFieldNotSupported("protocol", port.Protocol, supportedPortProtocols.List()))
 	}
 	return allErrs
 }

pkg/apis/extensions/validation/validation.go

@@ -56,7 +56,7 @@ func validateHorizontalPodAutoscalerSpec(autoscaler extensions.HorizontalPodAuto
 	if refErrs := ValidateSubresourceReference(autoscaler.ScaleRef); len(refErrs) > 0 {
 		allErrs = append(allErrs, refErrs.Prefix("scaleRef")...)
 	} else if autoscaler.ScaleRef.Subresource != "scale" {
-		allErrs = append(allErrs, validation.NewFieldValueNotSupported("scaleRef.subresource", autoscaler.ScaleRef.Subresource, []string{"scale"}))
+		allErrs = append(allErrs, validation.NewFieldNotSupported("scaleRef.subresource", autoscaler.ScaleRef.Subresource, []string{"scale"}))
 	}
 	return allErrs
 }
@@ -206,7 +206,7 @@ func ValidateDaemonSetSpec(spec *extensions.DaemonSetSpec) validation.ErrorList
 	allErrs = append(allErrs, apivalidation.ValidateReadOnlyPersistentDisks(spec.Template.Spec.Volumes).Prefix("template.spec.volumes")...)
 	// RestartPolicy has already been first-order validated as per ValidatePodTemplateSpec().
 	if spec.Template.Spec.RestartPolicy != api.RestartPolicyAlways {
-		allErrs = append(allErrs, validation.NewFieldValueNotSupported("template.spec.restartPolicy", spec.Template.Spec.RestartPolicy, []string{string(api.RestartPolicyAlways)}))
+		allErrs = append(allErrs, validation.NewFieldNotSupported("template.spec.restartPolicy", spec.Template.Spec.RestartPolicy, []string{string(api.RestartPolicyAlways)}))
 	}
 
 	return allErrs
@@ -360,7 +360,7 @@ func ValidateJobSpec(spec *extensions.JobSpec) validation.ErrorList {
 	allErrs = append(allErrs, apivalidation.ValidatePodTemplateSpec(&spec.Template).Prefix("template")...)
 	if spec.Template.Spec.RestartPolicy != api.RestartPolicyOnFailure &&
 		spec.Template.Spec.RestartPolicy != api.RestartPolicyNever {
-		allErrs = append(allErrs, validation.NewFieldValueNotSupported("template.spec.restartPolicy",
+		allErrs = append(allErrs, validation.NewFieldNotSupported("template.spec.restartPolicy",
 			spec.Template.Spec.RestartPolicy, []string{string(api.RestartPolicyOnFailure), string(api.RestartPolicyNever)}))
 	}
 	return allErrs

pkg/util/validation/errors.go

@@ -25,6 +25,38 @@ import (
 	"github.com/davecgh/go-spew/spew"
 )
 
+// Error is an implementation of the 'error' interface, which represents a
+// validation error.
+type Error struct {
+	Type     ErrorType
+	Field    string
+	BadValue interface{}
+	Detail   string
+}
+
+var _ error = &Error{}
+
+// Error implements the error interface.
+func (v *Error) Error() string {
+	return fmt.Sprintf("%s: %s", v.Field, v.ErrorBody())
+}
+
+// ErrorBody returns the error message without the field name.  This is useful
+// for building nice-looking higher-level error reporting.
+func (v *Error) ErrorBody() string {
+	var s string
+	switch v.Type {
+	case ErrorTypeRequired, ErrorTypeTooLong:
+		s = spew.Sprintf("%s", v.Type)
+	default:
+		s = spew.Sprintf("%s '%+v'", v.Type, v.BadValue)
+	}
+	if len(v.Detail) != 0 {
+		s += fmt.Sprintf(", Details: %s", v.Detail)
+	}
+	return s
+}
+
 // 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.
@@ -32,30 +64,34 @@ type ErrorType string
 
 // TODO: These values are duplicated in api/types.go, but there's a circular dep.  Fix it.
 const (
-	// ErrorType is used to report failure to find a requested value
+	// ErrorTypeNotFound is used to report failure to find a requested value
-	// (e.g. looking up an ID).
+	// (e.g. looking up an ID).  See NewFieldNotFound.
 	ErrorTypeNotFound ErrorType = "FieldValueNotFound"
 	// ErrorTypeRequired is used to report required values that are not
-	// provided (e.g. empty strings, null values, or empty arrays).
+	// provided (e.g. empty strings, null values, or empty arrays).  See
+	// NewFieldRequired.
 	ErrorTypeRequired ErrorType = "FieldValueRequired"
 	// ErrorTypeDuplicate is used to report collisions of values that must be
-	// unique (e.g. unique IDs).
+	// unique (e.g. unique IDs).  See NewFieldDuplicate.
 	ErrorTypeDuplicate ErrorType = "FieldValueDuplicate"
 	// ErrorTypeInvalid is used to report malformed values (e.g. failed regex
-	// match).
+	// match, too long, out of bounds).  See NewFieldInvalid.
 	ErrorTypeInvalid ErrorType = "FieldValueInvalid"
-	// ErrorTypeNotSupported is used to report valid (as per formatting rules)
+	// ErrorTypeNotSupported is used to report unknown values for enumerated
-	// values that can not be handled (e.g. an enumerated string).
+	// fields (e.g. a list of valid values).  See NewFieldNotSupported.
 	ErrorTypeNotSupported ErrorType = "FieldValueNotSupported"
 	// ErrorTypeForbidden is used to report valid (as per formatting rules)
-	// values which would be accepted by some api instances, but which would invoke behavior
+	// values which would be accepted under some conditions, but which are not
-	// not permitted by this api instance (such as due to stricter security policy).
+	// permitted by the current conditions (such as security policy).  See
+	// NewFieldForbidden.
 	ErrorTypeForbidden ErrorType = "FieldValueForbidden"
-	// ErrorTypeTooLong is used to report that given value is too long.
+	// ErrorTypeTooLong is used to report that the given value is too long.
+	// This is similar to ErrorTypeInvalid, but the error will not include the
+	// too-long value.  See NewFieldTooLong.
 	ErrorTypeTooLong ErrorType = "FieldValueTooLong"
 )
 
-// String converts a ErrorType into its corresponding error message.
+// String converts a ErrorType into its corresponding canonical error message.
 func (t ErrorType) String() string {
 	switch t {
 	case ErrorTypeNotFound:
@@ -73,51 +109,40 @@ func (t ErrorType) String() string {
 	case ErrorTypeTooLong:
 		return "too long"
 	default:
-		panic(fmt.Sprintf("unrecognized validation type: %#v", t))
+		panic(fmt.Sprintf("unrecognized validation error: %q", t))
 		return ""
 	}
 }
 
-// Error is an implementation of the 'error' interface, which represents an error of validation.
+// NewFieldNotFound returns a *Error indicating "value not found".  This is
-type Error struct {
+// used to report failure to find a requested value (e.g. looking up an ID).
-	Type     ErrorType
+func NewFieldNotFound(field string, value interface{}) *Error {
-	Field    string
+	return &Error{ErrorTypeNotFound, field, value, ""}
-	BadValue interface{}
-	Detail   string
 }
 
-var _ error = &Error{}
+// NewFieldRequired returns a *Error indicating "value required".  This is used
-
+// to report required values that are not provided (e.g. empty strings, null
-func (v *Error) Error() string {
+// values, or empty arrays).
-	return fmt.Sprintf("%s: %s", v.Field, v.ErrorBody())
-}
-
-func (v *Error) ErrorBody() string {
-	var s string
-	switch v.Type {
-	case ErrorTypeRequired, ErrorTypeTooLong:
-		s = spew.Sprintf("%s", v.Type)
-	default:
-		s = spew.Sprintf("%s '%+v'", v.Type, v.BadValue)
-	}
-	if len(v.Detail) != 0 {
-		s += fmt.Sprintf(", Details: %s", v.Detail)
-	}
-	return s
-}
-
-// NewFieldRequired returns a *Error indicating "value required"
 func NewFieldRequired(field string) *Error {
 	return &Error{ErrorTypeRequired, field, "", ""}
 }
 
-// NewFieldInvalid returns a *Error indicating "invalid value"
+// NewFieldDuplicate returns a *Error indicating "duplicate value".  This is
+// used to report collisions of values that must be unique (e.g. names or IDs).
+func NewFieldDuplicate(field string, value interface{}) *Error {
+	return &Error{ErrorTypeDuplicate, field, value, ""}
+}
+
+// NewFieldInvalid returns a *Error indicating "invalid value".  This is used
+// to report malformed values (e.g. failed regex match, too long, out of bounds).
 func NewFieldInvalid(field string, value interface{}, detail string) *Error {
 	return &Error{ErrorTypeInvalid, field, value, detail}
 }
 
-// NewFieldValueNotSupported returns a *Error indicating "unsupported value"
+// NewFieldNotSupported returns a *Error indicating "unsupported value".
-func NewFieldValueNotSupported(field string, value interface{}, validValues []string) *Error {
+// This is used to report unknown values for enumerated fields (e.g. a list of
+// valid values).
+func NewFieldNotSupported(field string, value interface{}, validValues []string) *Error {
 	detail := ""
 	if validValues != nil && len(validValues) > 0 {
 		detail = "supported values: " + strings.Join(validValues, ", ")
@@ -125,25 +150,23 @@ func NewFieldValueNotSupported(field string, value interface{}, validValues []st
 	return &Error{ErrorTypeNotSupported, field, value, detail}
 }
 
-// NewFieldForbidden returns a *Error indicating "forbidden"
+// NewFieldForbidden returns a *Error indicating "forbidden".  This is used to
+// report valid (as per formatting rules) values which would be accepted under
+// some conditions, but which are not permitted by current conditions (e.g.
+// security policy).
 func NewFieldForbidden(field string, value interface{}) *Error {
 	return &Error{ErrorTypeForbidden, field, value, ""}
 }
 
-// NewFieldDuplicate returns a *Error indicating "duplicate value"
+// NewFieldTooLong returns a *Error indicating "too long".  This is used to
-func NewFieldDuplicate(field string, value interface{}) *Error {
+// report that the given value is too long.  This is similar to
-	return &Error{ErrorTypeDuplicate, field, value, ""}
+// NewFieldInvalid, but the returned error will not include the too-long
-}
+// value.
-
-// NewFieldNotFound returns a *Error indicating "value not found"
-func NewFieldNotFound(field string, value interface{}) *Error {
-	return &Error{ErrorTypeNotFound, field, value, ""}
-}
-
 func NewFieldTooLong(field string, value interface{}, maxLength int) *Error {
 	return &Error{ErrorTypeTooLong, field, value, fmt.Sprintf("must have at most %d characters", maxLength)}
 }
 
+// ErrorList holds a set of errors.
 type ErrorList []error
 
 // Prefix adds a prefix to the Field of every Error in the list.

pkg/util/validation/errors_test.go

@@ -31,7 +31,7 @@ func TestMakeFuncs(t *testing.T) {
 			ErrorTypeInvalid,
 		},
 		{
-			func() *Error { return NewFieldValueNotSupported("f", "v", nil) },
+			func() *Error { return NewFieldNotSupported("f", "v", nil) },
 			ErrorTypeNotSupported,
 		},
 		{