Merge pull request #4898 from gmarek/client2

Loosen label and annotation validation and related tests

examples/guestbook/frontend-controller.json

@@ -21,7 +21,7 @@
        },
        "labels": {
          "name": "frontend",
-         "uses": "redis-slave,redis-master",
+         "uses": "redis-slave-or-redis-master",
          "app": "frontend"
        }
       }},

pkg/api/errors/validation.go

@@ -52,6 +52,8 @@ const (
 	// values which would be accepted by some api instances, but which would invoke behavior
 	// not permitted by this api instance (such as due to stricter security policy).
 	ValidationErrorTypeForbidden ValidationErrorType = "FieldValueForbidden"
+	// ValidationErrorTypeTooLong is used to report that given value is too long.
+	ValidationErrorTypeTooLong ValidationErrorType = "FieldValueTooLong"
 )
 
 // String converts a ValidationErrorType into its corresponding error message.
@@ -69,6 +71,8 @@ func (t ValidationErrorType) String() string {
 		return "unsupported value"
 	case ValidationErrorTypeForbidden:
 		return "forbidden"
+	case ValidationErrorTypeTooLong:
+		return "too long"
 	default:
 		glog.Errorf("unrecognized validation type: %#v", t)
 		return ""
@@ -130,6 +134,10 @@ func NewFieldNotFound(field string, value interface{}) *ValidationError {
 	return &ValidationError{ValidationErrorTypeNotFound, field, value, ""}
 }
 
+func NewFieldTooLong(field string, value interface{}) *ValidationError {
+	return &ValidationError{ValidationErrorTypeTooLong, field, value, ""}
+}
+
 type ValidationErrorList []error
 
 // Prefix adds a prefix to the Field of every ValidationError in the list.

pkg/api/validation/validation.go

@@ -30,7 +30,6 @@ import (
 	"github.com/golang/glog"
 )
 
-const qualifiedNameErrorMsg string = "must match regex [" + util.DNS1123SubdomainFmt + " / ] " + util.DNS1123LabelFmt
 const cIdentifierErrorMsg string = "must match regex " + util.CIdentifierFmt
 const isNegativeErrorMsg string = "value must not be negative"
 
@@ -38,19 +37,26 @@ func intervalErrorMsg(lo, hi int) string {
 	return fmt.Sprintf("must be greater than %d and less than %d", lo, hi)
 }
 
+var labelValueErrorMsg string = fmt.Sprintf("must have at most %d characters and match regex %s", util.LabelValueMaxLength, util.LabelValueFmt)
+var qualifiedNameErrorMsg string = fmt.Sprintf("must have at most %d characters and match regex %s", util.DNS1123SubdomainMaxLength, util.QualifiedNameFmt)
 var dnsSubdomainErrorMsg string = fmt.Sprintf("must have at most %d characters and match regex %s", util.DNS1123SubdomainMaxLength, util.DNS1123SubdomainFmt)
 var dnsLabelErrorMsg string = fmt.Sprintf("must have at most %d characters and match regex %s", util.DNS1123LabelMaxLength, util.DNS1123LabelFmt)
 var dns952LabelErrorMsg string = fmt.Sprintf("must have at most %d characters and match regex %s", util.DNS952LabelMaxLength, util.DNS952LabelFmt)
 var pdPartitionErrorMsg string = intervalErrorMsg(0, 255)
 var portRangeErrorMsg string = intervalErrorMsg(0, 65536)
 
+const totalAnnotationSizeLimitB int = 64 * (1 << 10) // 64 kB
+
 // ValidateLabels validates that a set of labels are correctly defined.
 func ValidateLabels(labels map[string]string, field string) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	for k := range labels {
+	for k, v := range labels {
 		if !util.IsQualifiedName(k) {
 			allErrs = append(allErrs, errs.NewFieldInvalid(field, k, qualifiedNameErrorMsg))
 		}
+		if !util.IsValidLabelValue(v) {
+			allErrs = append(allErrs, errs.NewFieldInvalid(field, v, labelValueErrorMsg))
+		}
 	}
 	return allErrs
 }
@@ -58,10 +64,18 @@ func ValidateLabels(labels map[string]string, field string) errs.ValidationError
 // ValidateAnnotations validates that a set of annotations are correctly defined.
 func ValidateAnnotations(annotations map[string]string, field string) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	for k := range annotations {
+	var totalSize int64
+	for k, v := range annotations {
 		if !util.IsQualifiedName(strings.ToLower(k)) {
 			allErrs = append(allErrs, errs.NewFieldInvalid(field, k, qualifiedNameErrorMsg))
 		}
+		if !util.IsValidAnnotationValue(v) {
+			allErrs = append(allErrs, errs.NewFieldInvalid(field, k, ""))
+		}
+		totalSize += (int64)(len(k)) + (int64)(len(v))
+	}
+	if totalSize > (int64)(totalAnnotationSizeLimitB) {
+		allErrs = append(allErrs, errs.NewFieldTooLong("annotations", ""))
 	}
 	return allErrs
 }

pkg/api/validation/validation_test.go

@@ -75,6 +75,8 @@ func TestValidateLabels(t *testing.T) {
 		{"1-num.2-num/3-num": "bar"},
 		{"1234/5678": "bar"},
 		{"1.2.3.4/5678": "bar"},
+		{"UpperCaseAreOK123": "bar"},
+		{"goodvalue": "123_-.BaR"},
 	}
 	for i := range successCases {
 		errs := ValidateLabels(successCases[i], "field")
@@ -83,15 +85,14 @@ func TestValidateLabels(t *testing.T) {
 		}
 	}
 
-	errorCases := []map[string]string{
+	labelNameErrorCases := []map[string]string{
-		{"NoUppercase123": "bar"},
 		{"nospecialchars^=@": "bar"},
 		{"cantendwithadash-": "bar"},
 		{"only/one/slash": "bar"},
 		{strings.Repeat("a", 254): "bar"},
 	}
-	for i := range errorCases {
+	for i := range labelNameErrorCases {
-		errs := ValidateLabels(errorCases[i], "field")
+		errs := ValidateLabels(labelNameErrorCases[i], "field")
 		if len(errs) != 1 {
 			t.Errorf("case[%d] expected failure", i)
 		} else {
@@ -101,6 +102,24 @@ func TestValidateLabels(t *testing.T) {
 			}
 		}
 	}
+
+	labelValueErrorCases := []map[string]string{
+		{"toolongvalue": strings.Repeat("a", 64)},
+		{"backslashesinvalue": "some\\bad\\value"},
+		{"nocommasallowed": "bad,value"},
+		{"strangecharsinvalue": "?#$notsogood"},
+	}
+	for i := range labelValueErrorCases {
+		errs := ValidateLabels(labelValueErrorCases[i], "field")
+		if len(errs) != 1 {
+			t.Errorf("case[%d] expected failure", i)
+		} else {
+			detail := errs[0].(*errors.ValidationError).Detail
+			if detail != labelValueErrorMsg {
+				t.Errorf("error detail %s should be equal %s", detail, labelValueErrorMsg)
+			}
+		}
+	}
 }
 
 func TestValidateAnnotations(t *testing.T) {
@@ -118,6 +137,11 @@ func TestValidateAnnotations(t *testing.T) {
 		{"1234/5678": "bar"},
 		{"1.2.3.4/5678": "bar"},
 		{"UpperCase123": "bar"},
+		{"a": strings.Repeat("b", (64*1024)-1)},
+		{
+			"a": strings.Repeat("b", (32*1024)-1),
+			"c": strings.Repeat("d", (32*1024)-1),
+		},
 	}
 	for i := range successCases {
 		errs := ValidateAnnotations(successCases[i], "field")
@@ -126,22 +150,34 @@ func TestValidateAnnotations(t *testing.T) {
 		}
 	}
 
-	errorCases := []map[string]string{
+	nameErrorCases := []map[string]string{
 		{"nospecialchars^=@": "bar"},
 		{"cantendwithadash-": "bar"},
 		{"only/one/slash": "bar"},
 		{strings.Repeat("a", 254): "bar"},
 	}
-	for i := range errorCases {
+	for i := range nameErrorCases {
-		errs := ValidateAnnotations(errorCases[i], "field")
+		errs := ValidateAnnotations(nameErrorCases[i], "field")
 		if len(errs) != 1 {
 			t.Errorf("case[%d] expected failure", i)
-		} else {
+		}
 		detail := errs[0].(*errors.ValidationError).Detail
 		if detail != qualifiedNameErrorMsg {
 			t.Errorf("error detail %s should be equal %s", detail, qualifiedNameErrorMsg)
 		}
 	}
+	totalSizeErrorCases := []map[string]string{
+		{"a": strings.Repeat("b", 64*1024)},
+		{
+			"a": strings.Repeat("b", 32*1024),
+			"c": strings.Repeat("d", 32*1024),
+		},
+	}
+	for i := range totalSizeErrorCases {
+		errs := ValidateAnnotations(totalSizeErrorCases[i], "field")
+		if len(errs) != 1 {
+			t.Errorf("case[%d] expected failure", i)
+		}
 	}
 }
 
@@ -840,19 +876,6 @@ func TestValidatePod(t *testing.T) {
 				DNSPolicy:     api.DNSClusterFirst,
 			},
 		},
-		"bad annotation": {
-			ObjectMeta: api.ObjectMeta{
-				Name:      "abc",
-				Namespace: "ns",
-				Annotations: map[string]string{
-					"NoUppercaseOrSpecialCharsLike=Equals": "bar",
-				},
-			},
-			Spec: api.PodSpec{
-				RestartPolicy: api.RestartPolicy{Always: &api.RestartPolicyAlways{}},
-				DNSPolicy:     api.DNSClusterFirst,
-			},
-		},
 	}
 	for k, v := range errorCases {
 		if errs := ValidatePod(&v); len(errs) == 0 {
@@ -1468,24 +1491,6 @@ func TestValidateService(t *testing.T) {
 			},
 			numErrs: 1,
 		},
-		{
-			name: "invalid annotation",
-			svc: api.Service{
-				ObjectMeta: api.ObjectMeta{
-					Name:      "abc123",
-					Namespace: api.NamespaceDefault,
-					Annotations: map[string]string{
-						"NoUppercaseOrSpecialCharsLike=Equals": "bar",
-					},
-				},
-				Spec: api.ServiceSpec{
-					Port:            8675,
-					Protocol:        "TCP",
-					SessionAffinity: "None",
-				},
-			},
-			numErrs: 1,
-		},
 		{
 			name: "invalid selector",
 			svc: api.Service{
@@ -1832,19 +1837,6 @@ func TestValidateReplicationController(t *testing.T) {
 				Template: &invalidPodTemplate.Spec,
 			},
 		},
-		"invalid_annotation": {
-			ObjectMeta: api.ObjectMeta{
-				Name:      "abc-123",
-				Namespace: api.NamespaceDefault,
-				Annotations: map[string]string{
-					"NoUppercaseOrSpecialCharsLike=Equals": "bar",
-				},
-			},
-			Spec: api.ReplicationControllerSpec{
-				Selector: validSelector,
-				Template: &validPodTemplate.Spec,
-			},
-		},
 		"invalid restart policy 1": {
 			ObjectMeta: api.ObjectMeta{
 				Name:      "abc-123",
@@ -1957,12 +1949,6 @@ func TestValidateMinion(t *testing.T) {
 				Labels: invalidSelector,
 			},
 		},
-		"invalid-annotations": {
-			ObjectMeta: api.ObjectMeta{
-				Name:        "abc-123",
-				Annotations: invalidSelector,
-			},
-		},
 	}
 	for k, v := range errorCases {
 		errs := ValidateMinion(&v)
@@ -2097,7 +2083,7 @@ func TestValidateMinionUpdate(t *testing.T) {
 				Name:   "foo",
 				Labels: map[string]string{"Foo": "baz"},
 			},
-		}, false},
+		}, true},
 	}
 	for i, test := range tests {
 		errs := ValidateMinionUpdate(&test.oldMinion, &test.minion)
@@ -2266,7 +2252,7 @@ func TestValidateServiceUpdate(t *testing.T) {
 					Name:   "foo",
 					Labels: map[string]string{"Foo": "baz"},
 				},
-			}, false},
+			}, true},
 	}
 	for i, test := range tests {
 		errs := ValidateServiceUpdate(&test.oldService, &test.service)
@@ -2300,7 +2286,7 @@ func TestValidateResourceNames(t *testing.T) {
 		{"my.favorite.app.co/_12345", false},
 		{"my.favorite.app.co/12345_", false},
 		{"kubernetes.io/..", false},
-		{"kubernetes.io/" + longString, false},
+		{"kubernetes.io/" + longString, true},
 		{"kubernetes.io//", false},
 		{"kubernetes.io", false},
 		{"kubernetes.io/will/not/work/", false},
@@ -2557,7 +2543,7 @@ func TestValidateNamespaceUpdate(t *testing.T) {
 				Name:   "foo",
 				Labels: map[string]string{"Foo": "baz"},
 			},
-		}, false},
+		}, true},
 	}
 	for i, test := range tests {
 		errs := ValidateNamespaceUpdate(&test.oldNamespace, &test.namespace)

pkg/labels/selector_test.go

@@ -417,7 +417,7 @@ func TestRequirementConstructor(t *testing.T) {
 		{"x", ExistsOperator, nil, true},
 		{"1foo", InOperator, util.NewStringSet("bar"), true},
 		{"1234", InOperator, util.NewStringSet("bar"), true},
-		{strings.Repeat("a", 64), ExistsOperator, nil, false}, //breaks DNS rule that len(key) <= 63
+		{strings.Repeat("a", 254), ExistsOperator, nil, false}, //breaks DNS rule that len(key) <= 253
 	}
 	for _, rc := range requirementConstructorTests {
 		if _, err := NewRequirement(rc.Key, rc.Op, rc.Vals); err == nil && !rc.Success {

pkg/util/validation.go

@@ -18,19 +18,33 @@ package util
 
 import (
 	"regexp"
-	"strings"
 )
 
-// IsDNSLabel tests for a string that conforms to the definition of a label in
+const kubeChar string = "[A-Za-z0-9]"
-// DNS (RFC 1123).
+const extendedKubeChar string = "[-A-Za-z0-9_.]"
-func IsDNSLabel(value string) bool {
+const qualifiedToken string = "(" + kubeChar + extendedKubeChar + "*)?" + kubeChar
-	return IsDNS1123Label(value)
+
+const LabelValueFmt string = "((" + kubeChar + extendedKubeChar + "*)?" + kubeChar + ")?"
+
+var labelValueRegexp = regexp.MustCompile("^" + LabelValueFmt + "$")
+
+const LabelValueMaxLength int = 63
+
+func IsValidLabelValue(value string) bool {
+	return (len(value) <= LabelValueMaxLength && labelValueRegexp.MatchString(value))
 }
 
-// IsDNSSubdomain tests for a string that conforms to the definition of a
+// Annotation values are opaque.
-// subdomain in DNS (RFC 1123).
+func IsValidAnnotationValue(value string) bool {
-func IsDNSSubdomain(value string) bool {
+	return true
-	return IsDNS1123Subdomain(value)
+}
+
+const QualifiedNameFmt string = "(" + qualifiedToken + "/)?" + qualifiedToken
+
+var qualifiedNameRegexp = regexp.MustCompile("^" + QualifiedNameFmt + "$")
+
+func IsQualifiedName(value string) bool {
+	return (len(value) <= DNS1123SubdomainMaxLength && qualifiedNameRegexp.MatchString(value))
 }
 
 const DNS1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"
@@ -57,6 +71,18 @@ func IsDNS1123Subdomain(value string) bool {
 	return len(value) <= DNS1123SubdomainMaxLength && dns1123SubdomainRegexp.MatchString(value)
 }
 
+// IsDNSLabel tests for a string that conforms to the definition of a label in
+// DNS (RFC 1123).
+func IsDNSLabel(value string) bool {
+	return IsDNS1123Label(value)
+}
+
+// IsDNSSubdomain tests for a string that conforms to the definition of a
+// subdomain in DNS (RFC 1123).
+func IsDNSSubdomain(value string) bool {
+	return IsDNS1123Subdomain(value)
+}
+
 const DNS952LabelFmt string = "[a-z]([-a-z0-9]*[a-z0-9])?"
 
 var dns952LabelRegexp = regexp.MustCompile("^" + DNS952LabelFmt + "$")
@@ -83,33 +109,3 @@ func IsCIdentifier(value string) bool {
 func IsValidPortNum(port int) bool {
 	return 0 < port && port < 65536
 }
-
-// IsQualifiedName tests whether a string fits the "optionally-namespaced
-// name" pattern: [ DNS_SUBDOMAIN "/" ] DNS_LABEL
-func IsQualifiedName(value string) bool {
-	var n, ns string
-	parts := strings.Split(value, "/")
-	switch len(parts) {
-	case 1:
-		n = parts[0]
-	case 2:
-		ns = parts[0]
-		n = parts[1]
-	default:
-		return false
-	}
-	if (ns != "" && !IsDNSSubdomain(ns)) || !IsDNSLabel(n) {
-		return false
-	}
-	return true
-}
-
-const LabelValueFmt string = "([A-Za-z0-9_\\-\\\\.]*)"
-
-var labelValueRegexp = regexp.MustCompile("^" + LabelValueFmt + "$")
-
-const labelValueMaxLength int = 63
-
-func IsValidLabelValue(value string) bool {
-	return (len(value) <= labelValueMaxLength && labelValueRegexp.MatchString(value))
-}

pkg/util/validation_test.go

@@ -168,6 +168,7 @@ func TestIsQualifiedName(t *testing.T) {
 		"1-num.2-num/3-num",
 		"1234/5678",
 		"1.2.3.4/5678",
+		"UppercaseIsOK123",
 	}
 	for i := range successCases {
 		if !IsQualifiedName(successCases[i]) {
@@ -176,12 +177,11 @@ func TestIsQualifiedName(t *testing.T) {
 	}
 
 	errorCases := []string{
-		"NoUppercase123",
 		"nospecialchars%^=@",
 		"cantendwithadash-",
-		"-cantstartwithadash",
 		"only/one/slash",
 		strings.Repeat("a", 254),
+		"-cantstartwithadash",
 	}
 	for i := range errorCases {
 		if IsQualifiedName(errorCases[i]) {
@@ -196,24 +196,25 @@ func TestIsValidLabelValue(t *testing.T) {
 		"now-with-dashes",
 		"1-starts-with-num",
 		"end-with-num-1",
-		"-starts-with-dash",
-		"ends-with-dash-",
-		".starts.with.dot",
-		"ends.with.dot.",
-		"\\preserve\\backslash",
 		"1234",                  // only num
 		strings.Repeat("a", 63), // to the limit
+		"", // empty value
 	}
 	for i := range successCases {
 		if !IsValidLabelValue(successCases[i]) {
-			t.Errorf("case[%d] expected success", i)
+			t.Errorf("case %s expected success", successCases[i])
 		}
 	}
 
 	errorCases := []string{
 		"nospecialchars%^=@",
 		"Tama-nui-te-rā.is.Māori.sun",
-		strings.Repeat("a", 65),
+		"\\backslashes\\are\\bad",
+		"-starts-with-dash",
+		"ends-with-dash-",
+		".starts.with.dot",
+		"ends.with.dot.",
+		strings.Repeat("a", 64), // over the limit
 	}
 	for i := range errorCases {
 		if IsValidLabelValue(errorCases[i]) {