From 77eff06a534744038fc445c45567db52794c3179 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 15 Dec 2015 23:49:58 -0800 Subject: [PATCH] Make IsDNS1123Label return error strings --- cluster/addons/dns/kube2sky/kube2sky.go | 2 +- pkg/api/validation/validation.go | 62 ++++++++++++-------- pkg/api/validation/validation_test.go | 18 +++--- pkg/apis/extensions/validation/validation.go | 10 ++-- pkg/kubelet/kubelet.go | 18 +++--- pkg/util/validation/validation.go | 57 ++++++++++++++---- pkg/util/validation/validation_test.go | 6 +- 7 files changed, 109 insertions(+), 64 deletions(-) diff --git a/cluster/addons/dns/kube2sky/kube2sky.go b/cluster/addons/dns/kube2sky/kube2sky.go index 0251d10b7f848..fe7dca5d411e0 100644 --- a/cluster/addons/dns/kube2sky/kube2sky.go +++ b/cluster/addons/dns/kube2sky/kube2sky.go @@ -208,7 +208,7 @@ func getHostname(address *kapi.EndpointAddress, podHostnames map[string]endpoint if len(address.Hostname) > 0 { return address.Hostname, true } - if hostRecord, exists := podHostnames[address.IP]; exists && validation.IsDNS1123Label(hostRecord.HostName) { + if hostRecord, exists := podHostnames[address.IP]; exists && len(validation.IsDNS1123Label(hostRecord.HostName)) == 0 { return hostRecord.HostName, true } return "", false diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index f165431400913..8e9a401ba3494 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -59,7 +59,6 @@ func InclusiveRangeErrorMsg(lo, hi int) string { } 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 = InclusiveRangeErrorMsg(1, 255) var PortRangeErrorMsg string = InclusiveRangeErrorMsg(1, 65535) @@ -113,12 +112,16 @@ func ValidatePodSpecificAnnotations(annotations map[string]string, fldPath *fiel allErrs = append(allErrs, ValidateTolerationsInPodAnnotations(annotations, fldPath)...) } - if hostname, exists := annotations[utilpod.PodHostnameAnnotation]; exists && !validation.IsDNS1123Label(hostname) { - allErrs = append(allErrs, field.Invalid(fldPath, utilpod.PodHostnameAnnotation, DNS1123LabelErrorMsg)) + if hostname, exists := annotations[utilpod.PodHostnameAnnotation]; exists { + for _, msg := range validation.IsDNS1123Label(hostname) { + allErrs = append(allErrs, field.Invalid(fldPath, utilpod.PodHostnameAnnotation, msg)) + } } - if subdomain, exists := annotations[utilpod.PodSubdomainAnnotation]; exists && !validation.IsDNS1123Label(subdomain) { - allErrs = append(allErrs, field.Invalid(fldPath, utilpod.PodSubdomainAnnotation, DNS1123LabelErrorMsg)) + if subdomain, exists := annotations[utilpod.PodSubdomainAnnotation]; exists { + for _, msg := range validation.IsDNS1123Label(subdomain) { + allErrs = append(allErrs, field.Invalid(fldPath, utilpod.PodSubdomainAnnotation, msg)) + } } return allErrs @@ -167,7 +170,7 @@ func ValidateOwnerReferences(ownerReferences []api.OwnerReference, fldPath *fiel // ValidateNameFunc validates that the provided name is valid for a given resource type. // Not all resources have the same validation rules for names. Prefix is true -// if the name will have a value appended to it. If the names is not valid, +// if the name will have a value appended to it. If the name is not valid, // this returns a list of descriptions of individual characteristics of the // value that were not valid. Otherwise this returns an empty list or nil. type ValidateNameFunc func(name string, prefix bool) []string @@ -249,10 +252,7 @@ func NameIsDNSLabel(name string, prefix bool) []string { if prefix { name = maskTrailingDash(name) } - if validation.IsDNS1123Label(name) { - return nil - } - return []string{DNS1123LabelErrorMsg} + return validation.IsDNS1123Label(name) } // NameIsDNS952Label is a ValidateNameFunc for names that must be a DNS 952 label. @@ -399,8 +399,10 @@ func validateVolumes(volumes []api.Volume, fldPath *field.Path) (sets.String, fi el := validateVolumeSource(&vol.VolumeSource, idxPath) if len(vol.Name) == 0 { el = append(el, field.Required(idxPath.Child("name"), "")) - } else if !validation.IsDNS1123Label(vol.Name) { - el = append(el, field.Invalid(idxPath.Child("name"), vol.Name, DNS1123LabelErrorMsg)) + } else if msgs := validation.IsDNS1123Label(vol.Name); len(msgs) != 0 { + for i := range msgs { + el = append(el, field.Invalid(idxPath.Child("name"), vol.Name, msgs[i])) + } } else if allNames.Has(vol.Name) { el = append(el, field.Duplicate(idxPath.Child("name"), vol.Name)) } @@ -1356,8 +1358,10 @@ func validateContainers(containers []api.Container, volumes sets.String, fldPath idxPath := fldPath.Index(i) if len(ctr.Name) == 0 { allErrs = append(allErrs, field.Required(idxPath.Child("name"), "")) - } else if !validation.IsDNS1123Label(ctr.Name) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ctr.Name, DNS1123LabelErrorMsg)) + } else if msgs := validation.IsDNS1123Label(ctr.Name); len(msgs) != 0 { + for i := range msgs { + allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ctr.Name, msgs[i])) + } } else if allNames.Has(ctr.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name)) } else { @@ -1547,12 +1551,16 @@ func ValidatePodSpec(spec *api.PodSpec, fldPath *field.Path) field.ErrorList { } } - if len(spec.Hostname) > 0 && !validation.IsDNS1123Label(spec.Hostname) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostname"), spec.Hostname, DNS1123LabelErrorMsg)) + if len(spec.Hostname) > 0 { + for _, msg := range validation.IsDNS1123Label(spec.Hostname) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostname"), spec.Hostname, msg)) + } } - if len(spec.Subdomain) > 0 && !validation.IsDNS1123Label(spec.Subdomain) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("subdomain"), spec.Subdomain, DNS1123LabelErrorMsg)) + if len(spec.Subdomain) > 0 { + for _, msg := range validation.IsDNS1123Label(spec.Subdomain) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("subdomain"), spec.Subdomain, msg)) + } } return allErrs @@ -2020,8 +2028,10 @@ func validateServicePort(sp *api.ServicePort, requireName, isHeadlessService boo if requireName && len(sp.Name) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) } else if len(sp.Name) != 0 { - if !validation.IsDNS1123Label(sp.Name) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), sp.Name, DNS1123LabelErrorMsg)) + if msgs := validation.IsDNS1123Label(sp.Name); len(msgs) != 0 { + for i := range msgs { + allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), sp.Name, msgs[i])) + } } else if allNames.Has(sp.Name) { allErrs = append(allErrs, field.Duplicate(fldPath.Child("name"), sp.Name)) } else { @@ -2892,8 +2902,10 @@ func validateEndpointAddress(address *api.EndpointAddress, fldPath *field.Path) if !validation.IsValidIP(address.IP) { allErrs = append(allErrs, field.Invalid(fldPath.Child("ip"), address.IP, "must be a valid IP address")) } - if len(address.Hostname) > 0 && !validation.IsDNS1123Label(address.Hostname) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("hostname"), address.Hostname, DNS1123LabelErrorMsg)) + if len(address.Hostname) > 0 { + for _, msg := range validation.IsDNS1123Label(address.Hostname) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostname"), address.Hostname, msg)) + } } if len(allErrs) > 0 { return allErrs @@ -2927,8 +2939,8 @@ func validateEndpointPort(port *api.EndpointPort, requireName bool, fldPath *fie if requireName && len(port.Name) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) } else if len(port.Name) != 0 { - if !validation.IsDNS1123Label(port.Name) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), port.Name, DNS1123LabelErrorMsg)) + for _, msg := range validation.IsDNS1123Label(port.Name) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), port.Name, msg)) } } if !validation.IsValidPortNum(int(port.Port)) { @@ -3024,7 +3036,7 @@ func isValidHostnamesMap(serializedPodHostNames string) bool { } for ip, hostRecord := range podHostNames { - if !validation.IsDNS1123Label(hostRecord.HostName) { + if len(validation.IsDNS1123Label(hostRecord.HostName)) != 0 { return false } if net.ParseIP(ip) == nil { diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index e78b615ed2e38..001ee1f30f641 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -90,10 +90,10 @@ func TestValidateObjectMetaNamespaces(t *testing.T) { return nil }, field.NewPath("field")) - if len(errs) != 1 { + if len(errs) != 2 { t.Fatalf("unexpected errors: %v", errs) } - if !strings.Contains(errs[0].Error(), "Invalid value") { + if !strings.Contains(errs[0].Error(), "Invalid value") || !strings.Contains(errs[1].Error(), "Invalid value") { t.Errorf("unexpected error message: %v", errs) } } @@ -748,12 +748,12 @@ func TestValidateVolumes(t *testing.T) { "name > 63 characters": { []api.Volume{{Name: strings.Repeat("a", 64), VolumeSource: emptyVS}}, field.ErrorTypeInvalid, - "name", "must be a DNS label", + "name", "must be no more than", }, "name not a DNS label": { []api.Volume{{Name: "a.b.c", VolumeSource: emptyVS}}, field.ErrorTypeInvalid, - "name", "must be a DNS label", + "name", "must match the regex", }, "name not unique": { []api.Volume{{Name: "abc", VolumeSource: emptyVS}, {Name: "abc", VolumeSource: emptyVS}}, @@ -4695,7 +4695,7 @@ func TestValidateLimitRange(t *testing.T) { }, "invalid-namespace": { api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "^Invalid"}, Spec: api.LimitRangeSpec{}}, - DNS1123LabelErrorMsg, + "must match the regex", }, "duplicate-limit-type": { api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{ @@ -4839,7 +4839,7 @@ func TestValidateLimitRange(t *testing.T) { } for i := range errs { detail := errs[i].Detail - if detail != v.D { + if !strings.Contains(detail, v.D) { t.Errorf("[%s]: expected error detail either empty or %q, got %q", k, v.D, detail) } } @@ -5019,7 +5019,7 @@ func TestValidateResourceQuota(t *testing.T) { }, "invalid Namespace": { api.ResourceQuota{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "^Invalid"}, Spec: spec}, - DNS1123LabelErrorMsg, + "must match the regex", }, "negative-limits": { api.ResourceQuota{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: negativeSpec}, @@ -5052,7 +5052,7 @@ func TestValidateResourceQuota(t *testing.T) { t.Errorf("expected failure for %s", k) } for i := range errs { - if errs[i].Detail != v.D { + if !strings.Contains(errs[i].Detail, v.D) { t.Errorf("[%s]: expected error detail either empty or %s, got %s", k, v.D, errs[i].Detail) } } @@ -5613,7 +5613,7 @@ func TestValidateEndpoints(t *testing.T) { "invalid namespace": { endpoints: api.Endpoints{ObjectMeta: api.ObjectMeta{Name: "mysvc", Namespace: "no@#invalid.;chars\"allowed"}}, errorType: "FieldValueInvalid", - errorDetail: DNS1123LabelErrorMsg, + errorDetail: "must match the regex", }, "invalid name": { endpoints: api.Endpoints{ObjectMeta: api.ObjectMeta{Name: "-_Invliad^&Characters", Namespace: "namespace"}}, diff --git a/pkg/apis/extensions/validation/validation.go b/pkg/apis/extensions/validation/validation.go index 5328ef10c3a1d..a9017780d22a0 100644 --- a/pkg/apis/extensions/validation/validation.go +++ b/pkg/apis/extensions/validation/validation.go @@ -69,8 +69,10 @@ func ValidateThirdPartyResource(obj *extensions.ThirdPartyResource) field.ErrorL version := &obj.Versions[ix] if len(version.Name) == 0 { allErrs = append(allErrs, field.Invalid(field.NewPath("versions").Index(ix).Child("name"), version, "must not be empty")) - } else if !validation.IsDNS1123Label(version.Name) { - allErrs = append(allErrs, field.Invalid(field.NewPath("versions").Index(ix).Child("name"), version, apivalidation.DNS1123LabelErrorMsg)) + } else { + for _, msg := range validation.IsDNS1123Label(version.Name) { + allErrs = append(allErrs, field.Invalid(field.NewPath("versions").Index(ix).Child("name"), version, msg)) + } } if versions.Has(version.Name) { allErrs = append(allErrs, field.Duplicate(field.NewPath("versions").Index(ix).Child("name"), version)) @@ -413,8 +415,8 @@ func validateIngressBackend(backend *extensions.IngressBackend, fldPath *field.P } } if backend.ServicePort.Type == intstr.String { - if !validation.IsDNS1123Label(backend.ServicePort.StrVal) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("servicePort"), backend.ServicePort.StrVal, apivalidation.DNS1123LabelErrorMsg)) + for _, msg := range validation.IsDNS1123Label(backend.ServicePort.StrVal) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("servicePort"), backend.ServicePort.StrVal, msg)) } if !validation.IsValidPortName(backend.ServicePort.StrVal) { allErrs = append(allErrs, field.Invalid(fldPath.Child("servicePort"), backend.ServicePort.StrVal, apivalidation.PortNameErrorMsg)) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 498846216460d..92cdac77d33b9 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -1312,14 +1312,13 @@ func (kl *Kubelet) GeneratePodHostNameAndDomain(pod *api.Pod) (string, string, e } hostname := pod.Name if len(pod.Spec.Hostname) > 0 { - if utilvalidation.IsDNS1123Label(pod.Spec.Hostname) { - hostname = pod.Spec.Hostname - } else { - return "", "", fmt.Errorf("Pod Hostname %q is not a valid DNS label.", pod.Spec.Hostname) + if msgs := utilvalidation.IsDNS1123Label(pod.Spec.Hostname); len(msgs) != 0 { + return "", "", fmt.Errorf("Pod Hostname %q is not a valid DNS label: %s", pod.Spec.Hostname, strings.Join(msgs, ";")) } + hostname = pod.Spec.Hostname } else { hostnameCandidate := podAnnotations[utilpod.PodHostnameAnnotation] - if utilvalidation.IsDNS1123Label(hostnameCandidate) { + if len(utilvalidation.IsDNS1123Label(hostnameCandidate)) == 0 { // use hostname annotation, if specified. hostname = hostnameCandidate } @@ -1331,14 +1330,13 @@ func (kl *Kubelet) GeneratePodHostNameAndDomain(pod *api.Pod) (string, string, e hostDomain := "" if len(pod.Spec.Subdomain) > 0 { - if utilvalidation.IsDNS1123Label(pod.Spec.Subdomain) { - hostDomain = fmt.Sprintf("%s.%s.svc.%s", pod.Spec.Subdomain, pod.Namespace, clusterDomain) - } else { - return "", "", fmt.Errorf("Pod Subdomain %q is not a valid DNS label.", pod.Spec.Subdomain) + if msgs := utilvalidation.IsDNS1123Label(pod.Spec.Subdomain); len(msgs) != 0 { + return "", "", fmt.Errorf("Pod Subdomain %q is not a valid DNS label: %s", pod.Spec.Subdomain, strings.Join(msgs, ";")) } + hostDomain = fmt.Sprintf("%s.%s.svc.%s", pod.Spec.Subdomain, pod.Namespace, clusterDomain) } else { subdomainCandidate := pod.Annotations[utilpod.PodSubdomainAnnotation] - if utilvalidation.IsDNS1123Label(subdomainCandidate) { + if len(utilvalidation.IsDNS1123Label(subdomainCandidate)) == 0 { hostDomain = fmt.Sprintf("%s.%s.svc.%s", subdomainCandidate, pod.Namespace, clusterDomain) } } diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index bc92290fb29d9..144ed4c4077cd 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -21,7 +21,6 @@ import ( "math" "net" "regexp" - "strconv" "strings" ) @@ -30,7 +29,6 @@ const qnameExtCharFmt string = "[-A-Za-z0-9_.]" const qualifiedNameFmt string = "(" + qnameCharFmt + qnameExtCharFmt + "*)?" + qnameCharFmt const qualifiedNameMaxLength int = 63 -var qualifiedNameMaxLengthString = strconv.Itoa(qualifiedNameMaxLength) var qualifiedNameRegexp = regexp.MustCompile("^" + qualifiedNameFmt + "$") // IsQualifiedName tests whether the value passed is what Kubernetes calls a @@ -48,21 +46,22 @@ func IsQualifiedName(value string) []string { var prefix string prefix, name = parts[0], parts[1] if len(prefix) == 0 { - errs = append(errs, "prefix part must be non-empty") + errs = append(errs, "prefix part "+EmptyError()) } else if !IsDNS1123Subdomain(prefix) { errs = append(errs, fmt.Sprintf("prefix part must be a DNS subdomain (e.g. 'example.com')")) } default: - return append(errs, "must match the regex "+qualifiedNameFmt+" with an optional DNS subdomain prefix and '/' (e.g. 'MyName' or 'example.com/MyName'") + return append(errs, RegexError(qualifiedNameFmt, "MyName", "my.name", "123-abc")+ + " with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName'") } if len(name) == 0 { - errs = append(errs, "name part must be non-empty") + errs = append(errs, "name part "+EmptyError()) } else if len(name) > qualifiedNameMaxLength { - errs = append(errs, "name part must be no more than "+qualifiedNameMaxLengthString+" characters") + errs = append(errs, "name part "+MaxLenError(qualifiedNameMaxLength)) } if !qualifiedNameRegexp.MatchString(name) { - errs = append(errs, "name part must match the regex "+qualifiedNameFmt+" (e.g. 'MyName' or 'my.name' or '123-abc')") + errs = append(errs, "name part "+RegexError(qualifiedNameFmt, "MyName", "my.name", "123-abc")) } return errs } @@ -70,7 +69,6 @@ func IsQualifiedName(value string) []string { const labelValueFmt string = "(" + qualifiedNameFmt + ")?" const LabelValueMaxLength int = 63 -var labelValueMaxLengthString = strconv.Itoa(LabelValueMaxLength) var labelValueRegexp = regexp.MustCompile("^" + labelValueFmt + "$") // IsValidLabelValue tests whether the value passed is a valid label value. If @@ -79,10 +77,10 @@ var labelValueRegexp = regexp.MustCompile("^" + labelValueFmt + "$") func IsValidLabelValue(value string) []string { var errs []string if len(value) > LabelValueMaxLength { - errs = append(errs, "must be no more than "+labelValueMaxLengthString+" characters") + errs = append(errs, MaxLenError(LabelValueMaxLength)) } if !labelValueRegexp.MatchString(value) { - errs = append(errs, "must match the regex "+labelValueFmt+" (e.g. 'MyValue' or 'my_value' or '12345')") + errs = append(errs, RegexError(labelValueFmt, "MyValue", "my_value", "12345")) } return errs } @@ -94,8 +92,15 @@ var dns1123LabelRegexp = regexp.MustCompile("^" + DNS1123LabelFmt + "$") // IsDNS1123Label tests for a string that conforms to the definition of a label in // DNS (RFC 1123). -func IsDNS1123Label(value string) bool { - return len(value) <= DNS1123LabelMaxLength && dns1123LabelRegexp.MatchString(value) +func IsDNS1123Label(value string) []string { + var errs []string + if len(value) > DNS1123LabelMaxLength { + errs = append(errs, MaxLenError(DNS1123LabelMaxLength)) + } + if !dns1123LabelRegexp.MatchString(value) { + errs = append(errs, RegexError(DNS1123LabelFmt, "my-name", "123-abc")) + } + return errs } const DNS1123SubdomainFmt string = DNS1123LabelFmt + "(\\." + DNS1123LabelFmt + ")*" @@ -206,3 +211,31 @@ var httpHeaderNameRegexp = regexp.MustCompile("^" + HTTPHeaderNameFmt + "$") func IsHTTPHeaderName(value string) bool { return httpHeaderNameRegexp.MatchString(value) } + +// MaxLenError returns a string explanation of a "string too long" validation +// failure. +func MaxLenError(length int) string { + return fmt.Sprintf("must be no more than %d characters", length) +} + +// RegexError returns a string explanation of a regex validation failure. +func RegexError(fmt string, examples ...string) string { + s := "must match the regex " + fmt + if len(examples) == 0 { + return s + } + s += " (e.g. " + for i := range examples { + if i > 0 { + s += " or " + } + s += "'" + examples[i] + "'" + } + return s + ")" +} + +// EmptyError returns a string explanation of a "must not be empty" validation +// failure. +func EmptyError() string { + return "must be non-empty" +} diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index d0b73e3b65b9c..f38ca7531fc5f 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -28,8 +28,8 @@ func TestIsDNS1123Label(t *testing.T) { strings.Repeat("a", 63), } for _, val := range goodValues { - if !IsDNS1123Label(val) { - t.Errorf("expected true for '%s'", val) + if msgs := IsDNS1123Label(val); len(msgs) != 0 { + t.Errorf("expected true for '%s': %v", val, msgs) } } @@ -42,7 +42,7 @@ func TestIsDNS1123Label(t *testing.T) { strings.Repeat("a", 64), } for _, val := range badValues { - if IsDNS1123Label(val) { + if msgs := IsDNS1123Label(val); len(msgs) == 0 { t.Errorf("expected false for '%s'", val) } }