Loosen label and annotation validation and related tests

2025-09-09 05:01:46 +00:00 · 2015-02-27 16:08:02 +01:00
parent 54b2b47caa
commit 726a5af075
6 changed files with 110 additions and 115 deletions
--- a/pkg/api/validation/validation.go
+++ b/pkg/api/validation/validation.go
@@ -30,7 +30,7 @@ import (
 	"github.com/golang/glog"
 )

-const qualifiedNameErrorMsg string = "must match regex [" + util.DNS1123SubdomainFmt + " / ] " + util.DNS1123LabelFmt
+const invalidAnnotationValueErrorMsg string = "must match regex " + util.AnnotationValueFmt
 const cIdentifierErrorMsg string = "must match regex " + util.CIdentifierFmt
 const isNegativeErrorMsg string = "value must not be negative"

@@ -38,6 +38,8 @@ 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)
@@ -47,10 +49,13 @@ var portRangeErrorMsg string = intervalErrorMsg(0, 65536)
 // 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 +63,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, invalidAnnotationValueErrorMsg))
+		}
+		totalSize += (int64)(len(k)) + (int64)(len(v))
+	}
+	if totalSize > (int64)(util.TotalAnnotationSizeB) {
+		allErrs = append(allErrs, errs.NewFieldTooLong("annotations", ""))
 	}
 	return allErrs
 }
@@ -868,7 +881,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")
 		}
 	}

--- a/pkg/api/validation/validation_test.go
+++ b/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{
-		{"NoUppercase123": "bar"},
+	labelNameErrorCases := []map[string]string{
 		{"nospecialchars^=@": "bar"},
 		{"cantendwithadash-": "bar"},
 		{"only/one/slash": "bar"},
 		{strings.Repeat("a", 254): "bar"},
 	}
-	for i := range errorCases {
-		errs := ValidateLabels(errorCases[i], "field")
+	for i := range labelNameErrorCases {
+		errs := ValidateLabels(labelNameErrorCases[i], "field")
 		if len(errs) != 1 {
 			t.Errorf("case[%d] expected failure", i)
 		} else {
@@ -101,6 +102,23 @@ func TestValidateLabels(t *testing.T) {
 			}
 		}
 	}
+
+	labelValueErrorCases := []map[string]string{
+		{"toolongvalue": strings.Repeat("a", 64)},
+		{"backslashesinvalue": "some\\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 +136,7 @@ func TestValidateAnnotations(t *testing.T) {
 		{"1234/5678": "bar"},
 		{"1.2.3.4/5678": "bar"},
 		{"UpperCase123": "bar"},
+		{"a": strings.Repeat("b", 64*(1<<10)-1)},
 	}
 	for i := range successCases {
 		errs := ValidateAnnotations(successCases[i], "field")
@@ -125,22 +144,30 @@ func TestValidateAnnotations(t *testing.T) {
 			t.Errorf("case[%d] expected success, got %#v", i, errs)
 		}
 	}
-
-	errorCases := []map[string]string{
+	
+	nameErrorCases := []map[string]string{
 		{"nospecialchars^=@": "bar"},
 		{"cantendwithadash-": "bar"},
 		{"only/one/slash": "bar"},
 		{strings.Repeat("a", 254): "bar"},
 	}
+	for i := range nameErrorCases {
+		errs := ValidateAnnotations(nameErrorCases[i], "field")
+		if len(errs) != 1 {
+			t.Errorf("case[%d] expected failure", i)
+		}
+		detail := errs[0].(*errors.ValidationError).Detail
+		if detail != qualifiedNameErrorMsg {
+			t.Errorf("error detail %s should be equal %s", detail, qualifiedNameErrorMsg)
+		}
+	}
+	errorCases := []map[string]string{
+		{"a": strings.Repeat("b", 64*(1<<10))},
+	}
 	for i := range errorCases {
 		errs := ValidateAnnotations(errorCases[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)
-			}
 		}
 	}
 }
@@ -840,19 +867,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 +1482,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 +1828,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 +1940,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 +2074,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 +2243,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)
@@ -2297,10 +2274,10 @@ func TestValidateResourceNames(t *testing.T) {
 		{".", false},
 		{"..", false},
 		{"my.favorite.app.co/12345", true},
-		{"my.favorite.app.co/_12345", false},
+		{"my.favorite.app.co/_12345", true},
 		{"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 +2534,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)