Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor validation.go to avoid duplicating #15850

Merged
merged 2 commits into from
Oct 25, 2015
Merged

refactor validation.go to avoid duplicating #15850

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57faaf4
refactor validation.go to avoid duplicating
mqliang Oct 12, 2015
ce384e6
fix conflict after rebase
mqliang Oct 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 34 additions & 35 deletions pkg/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ import (
// fields by default.
var RepairMalformedUpdates bool = true

const cIdentifierErrorMsg string = `must be a C identifier (matching regex ` + validation.CIdentifierFmt + `): e.g. "my_name" or "MyName"`
const isNegativeErrorMsg string = `must be non-negative`
const isNotIntegerErrorMsg string = `must be an integer`
const fieldImmutableErrorMsg string = `field is immutable`
const cIdentifierErrorMsg string = `must be a C identifier (matching regex ` + validation.CIdentifierFmt + `): e.g. "my_name" or "MyName"`
const isNotIntegerErrorMsg string = `must be an integer`

func intervalErrorMsg(lo, hi int) string {
func IntervalErrorMsg(lo, hi int) string {
return fmt.Sprintf(`must be greater than %d and less than %d`, lo, hi)
}

Expand All @@ -56,9 +56,9 @@ var qualifiedNameErrorMsg string = fmt.Sprintf(`must be a qualified name (at mos
var DNSSubdomainErrorMsg string = fmt.Sprintf(`must be a DNS subdomain (at most %d characters, matching regex %s): e.g. "example.com"`, validation.DNS1123SubdomainMaxLength, validation.DNS1123SubdomainFmt)
var DNS1123LabelErrorMsg string = fmt.Sprintf(`must be a DNS label (at most %d characters, matching regex %s): e.g. "my-name"`, validation.DNS1123LabelMaxLength, validation.DNS1123LabelFmt)
var DNS952LabelErrorMsg string = fmt.Sprintf(`must be a DNS 952 label (at most %d characters, matching regex %s): e.g. "my-name"`, validation.DNS952LabelMaxLength, validation.DNS952LabelFmt)
var pdPartitionErrorMsg string = intervalErrorMsg(0, 255)
var portRangeErrorMsg string = intervalErrorMsg(0, 65536)
var portNameErrorMsg string = fmt.Sprintf(`must be an IANA_SVC_NAME (at most 15 characters, matching regex %s, it must contain at least one letter [a-z], and hyphens cannot be adjacent to other hyphens): e.g. "http"`, validation.IdentifierNoHyphensBeginEndFmt)
var pdPartitionErrorMsg string = IntervalErrorMsg(0, 255)
var PortRangeErrorMsg string = IntervalErrorMsg(0, 65536)
var PortNameErrorMsg string = fmt.Sprintf(`must be an IANA_SVC_NAME (at most 15 characters, matching regex %s, it must contain at least one letter [a-z], and hyphens cannot be adjacent to other hyphens): e.g. "http"`, validation.IdentifierNoHyphensBeginEndFmt)

const totalAnnotationSizeLimitB int = 256 * (1 << 10) // 256 kB

Expand Down Expand Up @@ -235,6 +235,14 @@ func ValidatePositiveQuantity(value resource.Quantity, fieldName string) errs.Va
return allErrs
}

func ValidateImmutableField(old, new interface{}, fieldName string) errs.ValidationErrorList {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this can only return a single error, please have it just return error not errs.ValidationErrorList

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendandburns
From my perspective, returning errs.ValidationErrorList is a better choice, just as ValidatePositiveField and ValidatePositiveQuantity do. Those three validation functions can indeed only return a single error, but there are lots of other validation functions such as ValidateLabelName will return a list of error, keep all validation functions consistently return `errs.ValidationErrorList｀could make all invocation statements consistent.

allErrs := errs.ValidationErrorList{}
if !api.Semantic.DeepEqual(old, new) {
allErrs = append(allErrs, errs.NewFieldInvalid(fieldName, new, fieldImmutableErrorMsg))
}
return allErrs
}

// ValidateObjectMeta validates an object's metadata on creation. It expects that name generation has already
// been performed.
// TODO: Remove calls to this method scattered in validations of specific resources, e.g., ValidatePodUpdate.
Expand Down Expand Up @@ -315,18 +323,10 @@ func ValidateObjectMetaUpdate(new, old *api.ObjectMeta) errs.ValidationErrorList
allErrs = append(allErrs, errs.NewFieldInvalid("resourceVersion", new.ResourceVersion, "resourceVersion must be specified for an update"))
}

if old.Name != new.Name {
allErrs = append(allErrs, errs.NewFieldInvalid("name", new.Name, fieldImmutableErrorMsg))
}
if old.Namespace != new.Namespace {
allErrs = append(allErrs, errs.NewFieldInvalid("namespace", new.Namespace, fieldImmutableErrorMsg))
}
if old.UID != new.UID {
allErrs = append(allErrs, errs.NewFieldInvalid("uid", new.UID, fieldImmutableErrorMsg))
}
if old.CreationTimestamp != new.CreationTimestamp {
allErrs = append(allErrs, errs.NewFieldInvalid("creationTimestamp", new.CreationTimestamp, fieldImmutableErrorMsg))
}
allErrs = append(allErrs, ValidateImmutableField(old.Name, new.Name, "name")...)
allErrs = append(allErrs, ValidateImmutableField(old.Namespace, new.Namespace, "namespace")...)
allErrs = append(allErrs, ValidateImmutableField(old.UID, new.UID, "uid")...)
allErrs = append(allErrs, ValidateImmutableField(old.CreationTimestamp, new.CreationTimestamp, "creationTimestamp")...)

allErrs = append(allErrs, ValidateLabels(new.Labels, "labels")...)
allErrs = append(allErrs, ValidateAnnotations(new.Annotations, "annotations")...)
Expand Down Expand Up @@ -771,20 +771,20 @@ func validatePorts(ports []api.ContainerPort) errs.ValidationErrorList {
pErrs := errs.ValidationErrorList{}
if len(port.Name) > 0 {
if !validation.IsValidPortName(port.Name) {
pErrs = append(pErrs, errs.NewFieldInvalid("name", port.Name, portNameErrorMsg))
pErrs = append(pErrs, errs.NewFieldInvalid("name", port.Name, PortNameErrorMsg))
} else if allNames.Has(port.Name) {
pErrs = append(pErrs, errs.NewFieldDuplicate("name", port.Name))
} else {
allNames.Insert(port.Name)
}
}
if port.ContainerPort == 0 {
pErrs = append(pErrs, errs.NewFieldInvalid("containerPort", port.ContainerPort, portRangeErrorMsg))
pErrs = append(pErrs, errs.NewFieldInvalid("containerPort", port.ContainerPort, PortRangeErrorMsg))
} else if !validation.IsValidPortNum(port.ContainerPort) {
pErrs = append(pErrs, errs.NewFieldInvalid("containerPort", port.ContainerPort, portRangeErrorMsg))
pErrs = append(pErrs, errs.NewFieldInvalid("containerPort", port.ContainerPort, PortRangeErrorMsg))
}
if port.HostPort != 0 && !validation.IsValidPortNum(port.HostPort) {
pErrs = append(pErrs, errs.NewFieldInvalid("hostPort", port.HostPort, portRangeErrorMsg))
pErrs = append(pErrs, errs.NewFieldInvalid("hostPort", port.HostPort, PortRangeErrorMsg))
}
if len(port.Protocol) == 0 {
pErrs = append(pErrs, errs.NewFieldRequired("protocol"))
Expand Down Expand Up @@ -934,9 +934,9 @@ func validateHTTPGetAction(http *api.HTTPGetAction) errs.ValidationErrorList {
allErrors = append(allErrors, errs.NewFieldRequired("path"))
}
if http.Port.Kind == util.IntstrInt && !validation.IsValidPortNum(http.Port.IntVal) {
allErrors = append(allErrors, errs.NewFieldInvalid("port", http.Port, portRangeErrorMsg))
allErrors = append(allErrors, errs.NewFieldInvalid("port", http.Port, PortRangeErrorMsg))
} else if http.Port.Kind == util.IntstrString && !validation.IsValidPortName(http.Port.StrVal) {
allErrors = append(allErrors, errs.NewFieldInvalid("port", http.Port.StrVal, portNameErrorMsg))
allErrors = append(allErrors, errs.NewFieldInvalid("port", http.Port.StrVal, PortNameErrorMsg))
}
supportedSchemes := sets.NewString(string(api.URISchemeHTTP), string(api.URISchemeHTTPS))
if !supportedSchemes.Has(string(http.Scheme)) {
Expand All @@ -948,9 +948,9 @@ func validateHTTPGetAction(http *api.HTTPGetAction) errs.ValidationErrorList {
func validateTCPSocketAction(tcp *api.TCPSocketAction) errs.ValidationErrorList {
allErrors := errs.ValidationErrorList{}
if tcp.Port.Kind == util.IntstrInt && !validation.IsValidPortNum(tcp.Port.IntVal) {
allErrors = append(allErrors, errs.NewFieldInvalid("port", tcp.Port, portRangeErrorMsg))
allErrors = append(allErrors, errs.NewFieldInvalid("port", tcp.Port, PortRangeErrorMsg))
} else if tcp.Port.Kind == util.IntstrString && !validation.IsValidPortName(tcp.Port.StrVal) {
allErrors = append(allErrors, errs.NewFieldInvalid("port", tcp.Port.StrVal, portNameErrorMsg))
allErrors = append(allErrors, errs.NewFieldInvalid("port", tcp.Port.StrVal, PortNameErrorMsg))
}
return allErrors
}
Expand Down Expand Up @@ -1320,7 +1320,7 @@ func validateServicePort(sp *api.ServicePort, requireName bool, allNames *sets.S
}

if !validation.IsValidPortNum(sp.Port) {
allErrs = append(allErrs, errs.NewFieldInvalid("port", sp.Port, portRangeErrorMsg))
allErrs = append(allErrs, errs.NewFieldInvalid("port", sp.Port, PortRangeErrorMsg))
}

if len(sp.Protocol) == 0 {
Expand All @@ -1330,10 +1330,10 @@ func validateServicePort(sp *api.ServicePort, requireName bool, allNames *sets.S
}

if sp.TargetPort.Kind == util.IntstrInt && !validation.IsValidPortNum(sp.TargetPort.IntVal) {
allErrs = append(allErrs, errs.NewFieldInvalid("targetPort", sp.TargetPort, portRangeErrorMsg))
allErrs = append(allErrs, errs.NewFieldInvalid("targetPort", sp.TargetPort, PortRangeErrorMsg))
}
if sp.TargetPort.Kind == util.IntstrString && !validation.IsValidPortName(sp.TargetPort.StrVal) {
allErrs = append(allErrs, errs.NewFieldInvalid("targetPort", sp.TargetPort, portNameErrorMsg))
allErrs = append(allErrs, errs.NewFieldInvalid("targetPort", sp.TargetPort, PortNameErrorMsg))
}

return allErrs
Expand All @@ -1344,8 +1344,8 @@ func ValidateServiceUpdate(oldService, service *api.Service) errs.ValidationErro
allErrs := errs.ValidationErrorList{}
allErrs = append(allErrs, ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta).Prefix("metadata")...)

if api.IsServiceIPSet(oldService) && service.Spec.ClusterIP != oldService.Spec.ClusterIP {
allErrs = append(allErrs, errs.NewFieldInvalid("spec.clusterIP", service.Spec.ClusterIP, fieldImmutableErrorMsg))
if api.IsServiceIPSet(oldService) {
allErrs = append(allErrs, ValidateImmutableField(oldService.Spec.ClusterIP, service.Spec.ClusterIP, "spec.clusterIP")...)
}

allErrs = append(allErrs, ValidateService(service)...)
Expand Down Expand Up @@ -1710,9 +1710,8 @@ func ValidateSecretUpdate(oldSecret, newSecret *api.Secret) errs.ValidationError
if len(newSecret.Type) == 0 {
newSecret.Type = oldSecret.Type
}
if newSecret.Type != oldSecret.Type {
allErrs = append(allErrs, errs.NewFieldInvalid("type", newSecret.Type, fieldImmutableErrorMsg))
}

allErrs = append(allErrs, ValidateImmutableField(oldSecret.Type, newSecret.Type, "type")...)

allErrs = append(allErrs, ValidateSecret(newSecret)...)
return allErrs
Expand Down Expand Up @@ -1966,7 +1965,7 @@ func validateEndpointPort(port *api.EndpointPort, requireName bool) errs.Validat
}
}
if !validation.IsValidPortNum(port.Port) {
allErrs = append(allErrs, errs.NewFieldInvalid("port", port.Port, portRangeErrorMsg))
allErrs = append(allErrs, errs.NewFieldInvalid("port", port.Port, PortRangeErrorMsg))
}
if len(port.Protocol) == 0 {
allErrs = append(allErrs, errs.NewFieldRequired("protocol"))
Expand Down
16 changes: 8 additions & 8 deletions pkg/api/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,16 +607,16 @@ func TestValidatePorts(t *testing.T) {
F string
D string
}{
"name > 15 characters": {[]api.ContainerPort{{Name: strings.Repeat("a", 16), ContainerPort: 80, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].name", portNameErrorMsg},
"name not a IANA svc name ": {[]api.ContainerPort{{Name: "a.b.c", ContainerPort: 80, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].name", portNameErrorMsg},
"name not a IANA svc name (i.e. a number)": {[]api.ContainerPort{{Name: "80", ContainerPort: 80, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].name", portNameErrorMsg},
"name > 15 characters": {[]api.ContainerPort{{Name: strings.Repeat("a", 16), ContainerPort: 80, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].name", PortNameErrorMsg},
"name not a IANA svc name ": {[]api.ContainerPort{{Name: "a.b.c", ContainerPort: 80, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].name", PortNameErrorMsg},
"name not a IANA svc name (i.e. a number)": {[]api.ContainerPort{{Name: "80", ContainerPort: 80, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].name", PortNameErrorMsg},
"name not unique": {[]api.ContainerPort{
{Name: "abc", ContainerPort: 80, Protocol: "TCP"},
{Name: "abc", ContainerPort: 81, Protocol: "TCP"},
}, errors.ValidationErrorTypeDuplicate, "[1].name", ""},
"zero container port": {[]api.ContainerPort{{ContainerPort: 0, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].containerPort", portRangeErrorMsg},
"invalid container port": {[]api.ContainerPort{{ContainerPort: 65536, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].containerPort", portRangeErrorMsg},
"invalid host port": {[]api.ContainerPort{{ContainerPort: 80, HostPort: 65536, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].hostPort", portRangeErrorMsg},
"zero container port": {[]api.ContainerPort{{ContainerPort: 0, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].containerPort", PortRangeErrorMsg},
"invalid container port": {[]api.ContainerPort{{ContainerPort: 65536, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].containerPort", PortRangeErrorMsg},
"invalid host port": {[]api.ContainerPort{{ContainerPort: 80, HostPort: 65536, Protocol: "TCP"}}, errors.ValidationErrorTypeInvalid, "[0].hostPort", PortRangeErrorMsg},
"invalid protocol case": {[]api.ContainerPort{{ContainerPort: 80, Protocol: "tcp"}}, errors.ValidationErrorTypeNotSupported, "[0].protocol", "supported values: TCP, UDP"},
"invalid protocol": {[]api.ContainerPort{{ContainerPort: 80, Protocol: "ICMP"}}, errors.ValidationErrorTypeNotSupported, "[0].protocol", "supported values: TCP, UDP"},
"protocol required": {[]api.ContainerPort{{Name: "abc", ContainerPort: 80}}, errors.ValidationErrorTypeRequired, "[0].protocol", ""},
Expand Down Expand Up @@ -3831,7 +3831,7 @@ func TestValidateEndpoints(t *testing.T) {
},
},
errorType: "FieldValueInvalid",
errorDetail: portRangeErrorMsg,
errorDetail: PortRangeErrorMsg,
},
"Invalid protocol": {
endpoints: api.Endpoints{
Expand Down Expand Up @@ -3869,7 +3869,7 @@ func TestValidateEndpoints(t *testing.T) {
},
},
errorType: "FieldValueInvalid",
errorDetail: portRangeErrorMsg,
errorDetail: PortRangeErrorMsg,
},
"Port missing protocol": {
endpoints: api.Endpoints{
Expand Down
34 changes: 9 additions & 25 deletions pkg/apis/extensions/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,6 @@ import (
utilvalidation "k8s.io/kubernetes/pkg/util/validation"
)

const isNegativeErrorMsg string = `must be non-negative`

// TODO: Expose from apivalidation instead of duplicating.
func intervalErrorMsg(lo, hi int) string {
return fmt.Sprintf(`must be greater than %d and less than %d`, lo, hi)
}

var portRangeErrorMsg string = intervalErrorMsg(0, 65536)
var portNameErrorMsg string = fmt.Sprintf(`must be an IANA_SVC_NAME (at most 15 characters, matching regex %s, it must contain at least one letter [a-z], and hyphens cannot be adjacent to other hyphens): e.g. "http"`, validation.IdentifierNoHyphensBeginEndFmt)

// ValidateHorizontalPodAutoscaler can be used to check whether the given autoscaler name is valid.
// Prefix indicates this name will be used as part of generation, in which case trailing dashes are allowed.
func ValidateHorizontalPodAutoscalerName(name string, prefix bool) (bool, string) {
Expand Down Expand Up @@ -318,11 +308,11 @@ func ValidateJob(job *extensions.Job) errs.ValidationErrorList {
func ValidateJobSpec(spec *extensions.JobSpec) errs.ValidationErrorList {
allErrs := errs.ValidationErrorList{}

if spec.Parallelism != nil && *spec.Parallelism < 0 {
allErrs = append(allErrs, errs.NewFieldInvalid("parallelism", spec.Parallelism, isNegativeErrorMsg))
if spec.Parallelism != nil {
allErrs = append(allErrs, apivalidation.ValidatePositiveField(int64(*spec.Parallelism), "parallelism")...)
}
if spec.Completions != nil && *spec.Completions < 0 {
allErrs = append(allErrs, errs.NewFieldInvalid("completions", spec.Completions, isNegativeErrorMsg))
if spec.Completions != nil {
allErrs = append(allErrs, apivalidation.ValidatePositiveField(int64(*spec.Completions), "completions")...)
}
if spec.Selector == nil {
allErrs = append(allErrs, errs.NewFieldRequired("selector"))
Expand Down Expand Up @@ -371,15 +361,9 @@ func ValidateJobUpdateStatus(oldJob, job *extensions.Job) errs.ValidationErrorLi
func ValidateJobSpecUpdate(oldSpec, spec extensions.JobSpec) errs.ValidationErrorList {
allErrs := errs.ValidationErrorList{}
allErrs = append(allErrs, ValidateJobSpec(&spec)...)
if !api.Semantic.DeepEqual(oldSpec.Completions, spec.Completions) {
allErrs = append(allErrs, errs.NewFieldInvalid("completions", spec.Completions, "field is immutable"))
}
if !api.Semantic.DeepEqual(oldSpec.Selector, spec.Selector) {
allErrs = append(allErrs, errs.NewFieldInvalid("selector", spec.Selector, "field is immutable"))
}
if !api.Semantic.DeepEqual(oldSpec.Template, spec.Template) {
allErrs = append(allErrs, errs.NewFieldInvalid("template", "[omitted]", "field is immutable"))
}
allErrs = append(allErrs, apivalidation.ValidateImmutableField(oldSpec.Completions, spec.Completions, "completions")...)
allErrs = append(allErrs, apivalidation.ValidateImmutableField(oldSpec.Selector, spec.Selector, "selector")...)
allErrs = append(allErrs, apivalidation.ValidateImmutableField(oldSpec.Template, spec.Template, "template")...)
return allErrs
}

Expand Down Expand Up @@ -506,10 +490,10 @@ func validateIngressBackend(backend *extensions.IngressBackend) errs.ValidationE
allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort.StrVal, apivalidation.DNS1123LabelErrorMsg))
}
if !utilvalidation.IsValidPortName(backend.ServicePort.StrVal) {
allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort.StrVal, portNameErrorMsg))
allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort.StrVal, apivalidation.PortNameErrorMsg))
}
} else if !utilvalidation.IsValidPortNum(backend.ServicePort.IntVal) {
allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort, portRangeErrorMsg))
allErrs = append(allErrs, errs.NewFieldInvalid("servicePort", backend.ServicePort, apivalidation.PortRangeErrorMsg))
}
return allErrs
}
Expand Down