Merge pull request #375 from thockin/valid/dns_lengths

Simplify DNS validation checks

pkg/api/types.go

@@ -31,19 +31,17 @@ import (
 //     This defines the format, but not the length restriction, which should be
 //     specified at the definition of any field of this type.
 //
-// DNS_LABEL:  This is a string that conforms to the definition of a "label"
+// DNS_LABEL:  This is a string, no more than 63 characters long, that conforms
-//     in RFCs 1035 and 1123.  This is captured by the following regex:
+//     to the definition of a "label" in RFCs 1035 and 1123.  This is captured
+//     by the following regex:
 //         [a-z0-9]([-a-z0-9]*[a-z0-9])?
-//     This defines the format, but not the length restriction, which should be
-//     specified at the definition of any field of this type.
 //
-//  DNS_SUBDOMAIN:  This is a string that conforms to the definition of a
+//  DNS_SUBDOMAIN:  This is a string, no more than 253 characters long, that conforms
-//      "subdomain" in RFCs 1035 and 1123.  This is captured by the following regex:
+//      to the definition of a "subdomain" in RFCs 1035 and 1123.  This is captured
+//      by the following regex:
 //         [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*
 //     or more simply:
 //         DNS_LABEL(\.DNS_LABEL)*
-//     This defines the format, but not the length restriction, which should be
-//     specified at the definition of any field of this type.
 
 // ContainerManifest corresponds to the Container Manifest format, documented at:
 // https://developers.google.com/compute/docs/containers/container_vms#container_manifest
@@ -51,7 +49,7 @@ import (
 type ContainerManifest struct {
 	// Required: This must be a supported version string, such as "v1beta1".
 	Version string `yaml:"version" json:"version"`
-	// Required: This must be a DNS_SUBDOMAIN, 255 characters or less.
+	// Required: This must be a DNS_SUBDOMAIN.
 	ID         string      `yaml:"id" json:"id"`
 	Volumes    []Volume    `yaml:"volumes" json:"volumes"`
 	Containers []Container `yaml:"containers" json:"containers"`
@@ -59,17 +57,18 @@ type ContainerManifest struct {
 
 // Volume represents a named volume in a pod that may be accessed by any containers in the pod.
 type Volume struct {
-	// Required: This must be a DNS_LABEL, 63 characters or less.  Each volume in a pod
+	// Required: This must be a DNS_LABEL.  Each volume in a pod must have
-	// must have a unique name.
+	// a unique name.
 	Name string `yaml:"name" json:"name"`
 }
 
 // Port represents a network port in a single container
 type Port struct {
-	// Optional: If specified, this must be a DNS_LABEL, 63 characters or less.  Each
+	// Optional: If specified, this must be a DNS_LABEL.  Each named port
-	// container in a pod must have a unique name.
+	// in a pod must have a unique name.
 	Name string `yaml:"name,omitempty" json:"name,omitempty"`
-	// Optional: Defaults to ContainerPort.
+	// Optional: Defaults to ContainerPort.  If specified, this must be a
+	// valid port number, 0 < x < 65536.
 	HostPort int `yaml:"hostPort,omitempty" json:"hostPort,omitempty"`
 	// Required: This must be a valid port number, 0 < x < 65536.
 	ContainerPort int `yaml:"containerPort" json:"containerPort"`
@@ -102,8 +101,8 @@ type EnvVar struct {
 
 // Container represents a single container that is expected to be run on the host.
 type Container struct {
-	// Required: This must be a DNS_LABEL, 63 characters or less.  Each container in a
+	// Required: This must be a DNS_LABEL.  Each container in a pod must
-	// pod must have a unique name.
+	// have a unique name.
 	Name string `yaml:"name" json:"name"`
 	// Required.
 	Image string `yaml:"image" json:"image"`

pkg/api/validation.go

@@ -65,7 +65,7 @@ func validateVolumes(volumes []Volume) (util.StringSet, error) {
 	allNames := util.StringSet{}
 	for i := range volumes {
 		vol := &volumes[i] // so we can set default values
-		if len(vol.Name) > 63 || !util.IsDNSLabel(vol.Name) {
+		if !util.IsDNSLabel(vol.Name) {
 			return util.StringSet{}, makeInvalidError("Volume.Name", vol.Name)
 		}
 		if allNames.Has(vol.Name) {
@@ -99,7 +99,7 @@ func validateContainers(containers []Container, volumes util.StringSet) error {
 	allNames := util.StringSet{}
 	for i := range containers {
 		ctr := &containers[i] // so we can set default values
-		if len(ctr.Name) > 63 || !util.IsDNSLabel(ctr.Name) {
+		if !util.IsDNSLabel(ctr.Name) {
 			return makeInvalidError("Container.Name", ctr.Name)
 		}
 		if allNames.Has(ctr.Name) {
@@ -130,7 +130,7 @@ func ValidateManifest(manifest *ContainerManifest) error {
 	if !supportedManifestVersions.Has(manifest.Version) {
 		return makeNotSupportedError("ContainerManifest.Version", manifest.Version)
 	}
-	if len(manifest.ID) > 255 || !util.IsDNSSubdomain(manifest.ID) {
+	if !util.IsDNSSubdomain(manifest.ID) {
 		return makeInvalidError("ContainerManifest.ID", manifest.ID)
 	}
 	allVolumes, err := validateVolumes(manifest.Volumes)

pkg/util/validation.go

@@ -20,25 +20,32 @@ import (
 	"regexp"
 )
 
-var dnsLabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
+const dnsLabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
+
 var dnsLabelRegexp *regexp.Regexp = regexp.MustCompile("^" + dnsLabelFmt + "$")
 
+const dnsLabelMaxLength int = 63
+
 // IsDNSLabel tests for a string that conforms to the definition of a label in
-// DNS (RFC 1035/1123).  This checks the format, but not the length.
+// DNS (RFC 1035/1123).
 func IsDNSLabel(value string) bool {
-	return dnsLabelRegexp.MatchString(value)
+	return len(value) <= dnsLabelMaxLength && dnsLabelRegexp.MatchString(value)
 }
 
-var dnsSubdomainFmt string = dnsLabelFmt + "(\\." + dnsLabelFmt + ")*"
+const dnsSubdomainFmt string = dnsLabelFmt + "(\\." + dnsLabelFmt + ")*"
+
 var dnsSubdomainRegexp *regexp.Regexp = regexp.MustCompile("^" + dnsSubdomainFmt + "$")
 
+const dnsSubdomainMaxLength int = 253
+
 // IsDNSSubdomain tests for a string that conforms to the definition of a
-// subdomain in DNS (RFC 1035/1123).  This checks the format, but not the length.
+// subdomain in DNS (RFC 1035/1123).
 func IsDNSSubdomain(value string) bool {
-	return dnsSubdomainRegexp.MatchString(value)
+	return len(value) <= dnsSubdomainMaxLength && dnsSubdomainRegexp.MatchString(value)
 }
 
-var cIdentifierFmt string = "[A-Za-z_][A-Za-z0-9_]*"
+const cIdentifierFmt string = "[A-Za-z_][A-Za-z0-9_]*"
+
 var cIdentifierRegexp *regexp.Regexp = regexp.MustCompile("^" + cIdentifierFmt + "$")
 
 // IsCIdentifier tests for a string that conforms the definition of an identifier

pkg/util/validation_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -37,6 +38,7 @@ func TestIsDNSLabel(t *testing.T) {
 		"_", "a_", "_a", "a_b", "1_", "_1", "1_2",
 		".", "a.", ".a", "a.b", "1.", ".1", "1.2",
 		" ", "a ", " a", "a b", "1 ", " 1", "1 2",
+		strings.Repeat("a", 64),
 	}
 	for _, val := range badValues {
 		if IsDNSLabel(val) {
@@ -73,6 +75,7 @@ func TestIsDNSSubdomain(t *testing.T) {
 		"A.B.C.D.E", "AA.BB.CC.DD.EE", "a.B.c.d.e", "aa.bB.cc.dd.ee",
 		"a@b", "a,b", "a_b", "a;b",
 		"a:b", "a%b", "a?b", "a$b",
+		strings.Repeat("a", 254),
 	}
 	for _, val := range badValues {
 		if IsDNSSubdomain(val) {