Merge pull request #24799 from thockin/validation_pt8-2

Automatic merge from submit-queue

Make IsValidLabelValue return error strings

Part of the larger validation PR, broken out for easier review and merge.  Builds on previous PRs in the series.

contrib/mesos/pkg/node/node.go

@@ -181,12 +181,12 @@ func SlaveAttributesToLabels(attrs []*mesos.Attribute) map[string]string {
 		}
 
 		if errs := validation.IsQualifiedName(k); len(errs) != 0 {
-			log.V(3).Infof("ignoring invalid node label name %q", k, errs)
+			log.V(3).Infof("ignoring invalid node label %q: %v", k, errs)
 			continue
 		}
 
-		if !validation.IsValidLabelValue(v) {
-			log.V(3).Infof("ignoring invalid node label %s value: %q", k, v)
+		if errs := validation.IsValidLabelValue(v); len(errs) != 0 {
+			log.V(3).Infof("ignoring invalid node %s=%q: %v", k, v, errs)
 			continue
 		}
 

pkg/api/unversioned/validation/validation.go

@@ -17,17 +17,11 @@ limitations under the License.
 package validation
 
 import (
-	"fmt"
-
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/util/validation"
 	"k8s.io/kubernetes/pkg/util/validation/field"
 )
 
-var (
-	labelValueErrorMsg string = fmt.Sprintf(`must have at most %d characters, matching regex %s: e.g. "MyValue" or ""`, validation.LabelValueMaxLength, validation.LabelValueFmt)
-)
-
 func ValidateLabelSelector(ps *unversioned.LabelSelector, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if ps == nil {
@@ -72,8 +66,8 @@ func ValidateLabels(labels map[string]string, fldPath *field.Path) field.ErrorLi
 	allErrs := field.ErrorList{}
 	for k, v := range labels {
 		allErrs = append(allErrs, ValidateLabelName(k, fldPath)...)
-		if !validation.IsValidLabelValue(v) {
-			allErrs = append(allErrs, field.Invalid(fldPath, v, labelValueErrorMsg))
+		for _, err := range validation.IsValidLabelValue(v) {
+			allErrs = append(allErrs, field.Invalid(fldPath, v, err))
 		}
 	}
 	return allErrs

pkg/api/unversioned/validation/validation_test.go

@@ -67,20 +67,22 @@ func TestValidateLabels(t *testing.T) {
 		}
 	}
 
-	labelValueErrorCases := []map[string]string{
-		{"toolongvalue": strings.Repeat("a", 64)},
-		{"backslashesinvalue": "some\\bad\\value"},
-		{"nocommasallowed": "bad,value"},
-		{"strangecharsinvalue": "?#$notsogood"},
+	labelValueErrorCases := []struct {
+		labels map[string]string
+		expect string
+	}{
+		{map[string]string{"toolongvalue": strings.Repeat("a", 64)}, "must be no more than"},
+		{map[string]string{"backslashesinvalue": "some\\bad\\value"}, "must match the regex"},
+		{map[string]string{"nocommasallowed": "bad,value"}, "must match the regex"},
+		{map[string]string{"strangecharsinvalue": "?#$notsogood"}, "must match the regex"},
 	}
 	for i := range labelValueErrorCases {
-		errs := ValidateLabels(labelValueErrorCases[i], field.NewPath("field"))
+		errs := ValidateLabels(labelValueErrorCases[i].labels, field.NewPath("field"))
 		if len(errs) != 1 {
-			t.Errorf("case[%d] expected failure", i)
+			t.Errorf("case[%d]: expected failure", i)
 		} else {
-			detail := errs[0].Detail
-			if detail != labelValueErrorMsg {
-				t.Errorf("error detail %s should be equal %s", detail, labelValueErrorMsg)
+			if !strings.Contains(errs[0].Detail, labelValueErrorCases[i].expect) {
+				t.Errorf("case[%d]: error details do not include %q: %q", i, labelValueErrorCases[i].expect, errs[0].Detail)
 			}
 		}
 	}

pkg/api/validation/validation.go

@@ -58,7 +58,6 @@ func InclusiveRangeErrorMsg(lo, hi int) string {
 	return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi)
 }
 
-var labelValueErrorMsg string = fmt.Sprintf(`must have at most %d characters, matching regex %s: e.g. "MyValue" or ""`, validation.LabelValueMaxLength, validation.LabelValueFmt)
 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)

pkg/kubectl/cmd/label.go

@@ -124,9 +124,12 @@ func parseLabels(spec []string) (map[string]string, []string, error) {
 	for _, labelSpec := range spec {
 		if strings.Index(labelSpec, "=") != -1 {
 			parts := strings.Split(labelSpec, "=")
-			if len(parts) != 2 || len(parts[1]) == 0 || !validation.IsValidLabelValue(parts[1]) {
+			if len(parts) != 2 || len(parts[1]) == 0 {
 				return nil, nil, fmt.Errorf("invalid label spec: %v", labelSpec)
 			}
+			if errs := validation.IsValidLabelValue(parts[1]); len(errs) != 0 {
+				return nil, nil, fmt.Errorf("invalid label value: %q: %s", labelSpec, strings.Join(errs, ";"))
+			}
 			labels[parts[0]] = parts[1]
 		} else if strings.HasSuffix(labelSpec, "-") {
 			remove = append(remove, labelSpec[:len(labelSpec)-1])

pkg/kubelet/network/plugins.go

@@ -120,8 +120,8 @@ func InitNetworkPlugin(plugins []NetworkPlugin, networkPluginName string, host H
 	allErrs := []error{}
 	for _, plugin := range plugins {
 		name := plugin.Name()
-		if len(validation.IsQualifiedName(name)) != 0 {
-			allErrs = append(allErrs, fmt.Errorf("network plugin has invalid name: %#v", plugin))
+		if errs := validation.IsQualifiedName(name); len(errs) != 0 {
+			allErrs = append(allErrs, fmt.Errorf("network plugin has invalid name: %q: %s", name, strings.Join(errs, ";")))
 			continue
 		}
 

pkg/labels/selector.go

@@ -764,8 +764,6 @@ func parse(selector string) (internalSelector, error) {
 	return internalSelector(items), err
 }
 
-var labelValueErrorMsg string = fmt.Sprintf(`must have at most %d characters, matching regex %s: e.g. "MyValue" or ""`, validation.LabelValueMaxLength, validation.LabelValueFmt)
-
 func validateLabelKey(k string) error {
 	if errs := validation.IsQualifiedName(k); len(errs) != 0 {
 		return fmt.Errorf("invalid label key %q: %s", k, strings.Join(errs, "; "))
@@ -774,8 +772,8 @@ func validateLabelKey(k string) error {
 }
 
 func validateLabelValue(v string) error {
-	if !validation.IsValidLabelValue(v) {
-		return fmt.Errorf("invalid label value: %s", labelValueErrorMsg)
+	if errs := validation.IsValidLabelValue(v); len(errs) != 0 {
+		return fmt.Errorf("invalid label value: %q: %s", v, strings.Join(errs, "; "))
 	}
 	return nil
 }

pkg/util/validation/validation.go

@@ -67,13 +67,24 @@ func IsQualifiedName(value string) []string {
 	return errs
 }
 
-const LabelValueFmt string = "(" + qualifiedNameFmt + ")?"
+const labelValueFmt string = "(" + qualifiedNameFmt + ")?"
 const LabelValueMaxLength int = 63
 
-var labelValueRegexp = regexp.MustCompile("^" + LabelValueFmt + "$")
+var labelValueMaxLengthString = strconv.Itoa(LabelValueMaxLength)
+var labelValueRegexp = regexp.MustCompile("^" + labelValueFmt + "$")
 
-func IsValidLabelValue(value string) bool {
-	return (len(value) <= LabelValueMaxLength && labelValueRegexp.MatchString(value))
+// IsValidLabelValue tests whether the value passed is a valid label value.  If
+// the value is not valid, a list of error strings is returned.  Otherwise an
+// empty list (or nil) is returned.
+func IsValidLabelValue(value string) []string {
+	var errs []string
+	if len(value) > LabelValueMaxLength {
+		errs = append(errs, "must be no more than "+labelValueMaxLengthString+" characters")
+	}
+	if !labelValueRegexp.MatchString(value) {
+		errs = append(errs, "must match the regex "+labelValueFmt+" (e.g. 'MyValue' or 'my_value' or '12345')")
+	}
+	return errs
 }
 
 const DNS1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?"

pkg/util/validation/validation_test.go

@@ -257,8 +257,8 @@ func TestIsValidLabelValue(t *testing.T) {
 		"", // empty value
 	}
 	for i := range successCases {
-		if !IsValidLabelValue(successCases[i]) {
-			t.Errorf("case %s expected success", successCases[i])
+		if errs := IsValidLabelValue(successCases[i]); len(errs) != 0 {
+			t.Errorf("case %s expected success: %v", successCases[i], errs)
 		}
 	}
 
@@ -273,7 +273,7 @@ func TestIsValidLabelValue(t *testing.T) {
 		strings.Repeat("a", 64), // over the limit
 	}
 	for i := range errorCases {
-		if IsValidLabelValue(errorCases[i]) {
+		if errs := IsValidLabelValue(errorCases[i]); len(errs) == 0 {
 			t.Errorf("case[%d] expected failure", i)
 		}
 	}

pkg/volume/plugins.go

@@ -272,8 +272,8 @@ func (pm *VolumePluginMgr) InitPlugins(plugins []VolumePlugin, host VolumeHost)
 	allErrs := []error{}
 	for _, plugin := range plugins {
 		name := plugin.Name()
-		if len(validation.IsQualifiedName(name)) != 0 {
-			allErrs = append(allErrs, fmt.Errorf("volume plugin has invalid name: %#v", plugin))
+		if errs := validation.IsQualifiedName(name); len(errs) != 0 {
+			allErrs = append(allErrs, fmt.Errorf("volume plugin has invalid name: %q: %s", name, strings.Join(errs, ";")))
 			continue
 		}
 

plugin/pkg/scheduler/factory/factory.go

@@ -307,8 +307,8 @@ func (f *ConfigFactory) CreateFromKeys(predicateKeys, priorityKeys sets.String,
 
 	failureDomainArgs := strings.Split(f.FailureDomains, ",")
 	for _, failureDomain := range failureDomainArgs {
-		if len(utilvalidation.IsQualifiedName(failureDomain)) != 0 {
-			return nil, fmt.Errorf("invalid failure domain: %s", failureDomain)
+		if errs := utilvalidation.IsQualifiedName(failureDomain); len(errs) != 0 {
+			return nil, fmt.Errorf("invalid failure domain: %q: %s", failureDomain, strings.Join(errs, ";"))
 		}
 	}