apply comments

duda · Mar 6, 2015 · bb8a4f5 · bb8a4f5
1 parent 726a5af
commit bb8a4f5
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 24 deletions.
diff --git a/examples/guestbook/frontend-controller.json b/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"
        }
       }},

diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go
@@ -30,7 +30,6 @@ import (
 	"github.com/golang/glog"
 )
 
-const invalidAnnotationValueErrorMsg string = "must match regex " + util.AnnotationValueFmt
 const cIdentifierErrorMsg string = "must match regex " + util.CIdentifierFmt
 const isNegativeErrorMsg string = "value must not be negative"
 
@@ -46,6 +45,8 @@ var dns952LabelErrorMsg string = fmt.Sprintf("must have at most %d characters an
 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{}
@@ -69,11 +70,11 @@ func ValidateAnnotations(annotations map[string]string, field string) errs.Valid
 			allErrs = append(allErrs, errs.NewFieldInvalid(field, k, qualifiedNameErrorMsg))
 		}
 		if !util.IsValidAnnotationValue(v) {
-			allErrs = append(allErrs, errs.NewFieldInvalid(field, k, invalidAnnotationValueErrorMsg))
+			allErrs = append(allErrs, errs.NewFieldInvalid(field, k, ""))
 		}
 		totalSize += (int64)(len(k)) + (int64)(len(v))
 	}
-	if totalSize > (int64)(util.TotalAnnotationSizeB) {
+	if totalSize > (int64)(totalAnnotationSizeLimitB) {
 		allErrs = append(allErrs, errs.NewFieldTooLong("annotations", ""))
 	}
 	return allErrs
@@ -881,7 +882,7 @@ func validateResourceName(value string, field string) errs.ValidationErrorList {
 
 	if len(strings.Split(value, "/")) == 1 {
 		if !api.IsStandardResourceName(value) {
-			return append(allErrs, errs.NewFieldInvalid(field, value, "is neither a standard resource type nor is fully qualified")
+			return append(allErrs, errs.NewFieldInvalid(field, value, "is neither a standard resource type nor is fully qualified"))
 		}
 	}
 

diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go
@@ -76,7 +76,7 @@ func TestValidateLabels(t *testing.T) {
 		{"1234/5678": "bar"},
 		{"1.2.3.4/5678": "bar"},
 		{"UpperCaseAreOK123": "bar"},
-		{"goodvalue": "123_-.BaR,"},
+		{"goodvalue": "123_-.BaR"},
 	}
 	for i := range successCases {
 		errs := ValidateLabels(successCases[i], "field")
@@ -106,6 +106,7 @@ 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 {
@@ -136,15 +137,19 @@ func TestValidateAnnotations(t *testing.T) {
 		{"1234/5678": "bar"},
 		{"1.2.3.4/5678": "bar"},
 		{"UpperCase123": "bar"},
-		{"a": strings.Repeat("b", 64*(1<<10)-1)},
+		{"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")
 		if len(errs) != 0 {
 			t.Errorf("case[%d] expected success, got %#v", i, errs)
 		}
 	}
-	
+
 	nameErrorCases := []map[string]string{
 		{"nospecialchars^=@": "bar"},
 		{"cantendwithadash-": "bar"},
@@ -161,11 +166,15 @@ func TestValidateAnnotations(t *testing.T) {
 			t.Errorf("error detail %s should be equal %s", detail, qualifiedNameErrorMsg)
 		}
 	}
-	errorCases := []map[string]string{
-		{"a": strings.Repeat("b", 64*(1<<10))},
+	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 errorCases {
-		errs := ValidateAnnotations(errorCases[i], "field")
+	for i := range totalSizeErrorCases {
+		errs := ValidateAnnotations(totalSizeErrorCases[i], "field")
 		if len(errs) != 1 {
 			t.Errorf("case[%d] expected failure", i)
 		}
@@ -2274,7 +2283,7 @@ func TestValidateResourceNames(t *testing.T) {
 		{".", false},
 		{"..", false},
 		{"my.favorite.app.co/12345", true},
-		{"my.favorite.app.co/_12345", true},
+		{"my.favorite.app.co/_12345", false},
 		{"my.favorite.app.co/12345_", false},
 		{"kubernetes.io/..", false},
 		{"kubernetes.io/" + longString, true},

diff --git a/pkg/labels/selector_test.go b/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, true}, //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 {

diff --git a/pkg/util/validation.go b/pkg/util/validation.go
@@ -20,7 +20,7 @@ import (
 	"regexp"
 )
 
-const LabelValueFmt string = "[A-Za-z0-9_\\-\\.,]*"
+const LabelValueFmt string = "[-A-Za-z0-9_.]*"
 
 var labelValueRegexp = regexp.MustCompile("^" + LabelValueFmt + "$")
 
@@ -30,17 +30,15 @@ func IsValidLabelValue(value string) bool {
 	return (len(value) <= LabelValueMaxLength && labelValueRegexp.MatchString(value))
 }
 
-const AnnotationValueFmt string = ".*"
-
-var annotationValueRegexp = regexp.MustCompile("^" + AnnotationValueFmt + "$")
-
-const TotalAnnotationSizeB int = 64 * (1 << 10) // 64 kB
-
+// Annotation values are opaque.
 func IsValidAnnotationValue(value string) bool {
-	return annotationValueRegexp.MatchString(value)
+	return true
 }
 
-const QualifiedNameFmt string = "([A-Za-z0-9_\\-\\.]+/)?[A-Za-z0-9_\\-\\.]*[A-Za-z0-9]"
+const kubeToken string = "[A-Za-z0-9]"
+const extendedKubeToken string = "[-A-Za-z0-9_.]"
+const qualifiedNamePiece string = "(" + kubeToken + extendedKubeToken + "*)?" + kubeToken
+const QualifiedNameFmt string = "(" + qualifiedNamePiece + "/)?" + qualifiedNamePiece
 
 var qualifiedNameRegexp = regexp.MustCompile("^" + QualifiedNameFmt + "$")
 

diff --git a/pkg/util/validation_test.go b/pkg/util/validation_test.go
@@ -169,7 +169,6 @@ func TestIsQualifiedName(t *testing.T) {
 		"1234/5678",
 		"1.2.3.4/5678",
 		"UppercaseIsOK123",
-		"-canstartwithadash",
 	}
 	for i := range successCases {
 		if !IsQualifiedName(successCases[i]) {
@@ -182,6 +181,7 @@ func TestIsQualifiedName(t *testing.T) {
 		"cantendwithadash-",
 		"only/one/slash",
 		strings.Repeat("a", 254),
+		"-cantstartwithadash",
 	}
 	for i := range errorCases {
 		if IsQualifiedName(errorCases[i]) {