Merge pull request #24798 from thockin/validation_pt8-1

Automatic merge from submit-queue

Make IsQualifiedName return error strings

Part of the larger validation PR, broken out for easier review and merge.

@lavalamp FYI, but I know you're swamped, too.

contrib/mesos/pkg/node/node.go

@@ -180,8 +180,8 @@ func SlaveAttributesToLabels(attrs []*mesos.Attribute) map[string]string {
 			v = strconv.FormatFloat(a.GetScalar().GetValue(), 'G', -1, 64)
 		}
 
-		if !validation.IsQualifiedName(k) {
+		if errs := validation.IsQualifiedName(k); len(errs) != 0 {
-			log.V(3).Infof("ignoring invalid node label name %q", k)
+			log.V(3).Infof("ignoring invalid node label name %q", k, errs)
 			continue
 		}
 

pkg/api/unversioned/validation/validation.go

@@ -25,8 +25,7 @@ import (
 )
 
 var (
-	labelValueErrorMsg    string = fmt.Sprintf(`must have at most %d characters, matching regex %s: e.g. "MyValue" or ""`, validation.LabelValueMaxLength, validation.LabelValueFmt)
+	labelValueErrorMsg string = fmt.Sprintf(`must have at most %d characters, matching regex %s: e.g. "MyValue" or ""`, validation.LabelValueMaxLength, validation.LabelValueFmt)
-	qualifiedNameErrorMsg string = fmt.Sprintf(`must be a qualified name (at most %d characters, matching regex %s), with an optional DNS subdomain prefix (at most %d characters, matching regex %s) and slash (/): e.g. "MyName" or "example.com/MyName"`, validation.QualifiedNameMaxLength, validation.QualifiedNameFmt, validation.DNS1123SubdomainMaxLength, validation.DNS1123SubdomainFmt)
 )
 
 func ValidateLabelSelector(ps *unversioned.LabelSelector, fldPath *field.Path) field.ErrorList {
@@ -62,8 +61,8 @@ func ValidateLabelSelectorRequirement(sr unversioned.LabelSelectorRequirement, f
 // ValidateLabelName validates that the label name is correctly defined.
 func ValidateLabelName(labelName string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if !validation.IsQualifiedName(labelName) {
+	for _, err := range validation.IsQualifiedName(labelName) {
-		allErrs = append(allErrs, field.Invalid(fldPath, labelName, qualifiedNameErrorMsg))
+		allErrs = append(allErrs, field.Invalid(fldPath, labelName, err))
 	}
 	return allErrs
 }

pkg/api/unversioned/validation/validation_test.go

@@ -47,20 +47,22 @@ func TestValidateLabels(t *testing.T) {
 		}
 	}
 
-	labelNameErrorCases := []map[string]string{
+	labelNameErrorCases := []struct {
-		{"nospecialchars^=@": "bar"},
+		labels map[string]string
-		{"cantendwithadash-": "bar"},
+		expect string
-		{"only/one/slash": "bar"},
+	}{
-		{strings.Repeat("a", 254): "bar"},
+		{map[string]string{"nospecialchars^=@": "bar"}, "must match the regex"},
+		{map[string]string{"cantendwithadash-": "bar"}, "must match the regex"},
+		{map[string]string{"only/one/slash": "bar"}, "must match the regex"},
+		{map[string]string{strings.Repeat("a", 254): "bar"}, "must be no more than"},
 	}
 	for i := range labelNameErrorCases {
-		errs := ValidateLabels(labelNameErrorCases[i], field.NewPath("field"))
+		errs := ValidateLabels(labelNameErrorCases[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 !strings.Contains(errs[0].Detail, labelNameErrorCases[i].expect) {
-			if detail != qualifiedNameErrorMsg {
+				t.Errorf("case[%d]: error details do not include %q: %q", i, labelNameErrorCases[i].expect, errs[0].Detail)
-				t.Errorf("error detail %s should be equal %s", detail, qualifiedNameErrorMsg)
 			}
 		}
 	}

pkg/api/validation/validation.go

@@ -59,7 +59,6 @@ func InclusiveRangeErrorMsg(lo, hi int) string {
 }
 
 var labelValueErrorMsg string = fmt.Sprintf(`must have at most %d characters, matching regex %s: e.g. "MyValue" or ""`, validation.LabelValueMaxLength, validation.LabelValueFmt)
-var qualifiedNameErrorMsg string = fmt.Sprintf(`must be a qualified name (at most %d characters, matching regex %s), with an optional DNS subdomain prefix (at most %d characters, matching regex %s) and slash (/): e.g. "MyName" or "example.com/MyName"`, validation.QualifiedNameMaxLength, validation.QualifiedNameFmt, validation.DNS1123SubdomainMaxLength, validation.DNS1123SubdomainFmt)
 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)
@@ -94,8 +93,8 @@ func ValidateAnnotations(annotations map[string]string, fldPath *field.Path) fie
 	allErrs := field.ErrorList{}
 	var totalSize int64
 	for k, v := range annotations {
-		if !validation.IsQualifiedName(strings.ToLower(k)) {
+		for _, err := range validation.IsQualifiedName(strings.ToLower(k)) {
-			allErrs = append(allErrs, field.Invalid(fldPath, k, qualifiedNameErrorMsg))
+			allErrs = append(allErrs, field.Invalid(fldPath, k, err))
 		}
 		totalSize += (int64)(len(k)) + (int64)(len(v))
 	}
@@ -2148,8 +2147,11 @@ func ValidateNodeUpdate(node, oldNode *api.Node) field.ErrorList {
 // Refer to docs/design/resources.md for more details.
 func validateResourceName(value string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if !validation.IsQualifiedName(value) {
+	for _, err := range validation.IsQualifiedName(value) {
-		return append(allErrs, field.Invalid(fldPath, value, qualifiedNameErrorMsg))
+		allErrs = append(allErrs, field.Invalid(fldPath, value, err))
+	}
+	if len(allErrs) != 0 {
+		return allErrs
 	}
 
 	if len(strings.Split(value, "/")) == 1 {
@@ -2188,8 +2190,11 @@ func validateResourceQuotaResourceName(value string, fldPath *field.Path) field.
 // Validate limit range types
 func validateLimitRangeTypeName(value string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if !validation.IsQualifiedName(value) {
+	for _, err := range validation.IsQualifiedName(value) {
-		return append(allErrs, field.Invalid(fldPath, value, qualifiedNameErrorMsg))
+		allErrs = append(allErrs, field.Invalid(fldPath, value, err))
+	}
+	if len(allErrs) != 0 {
+		return allErrs
 	}
 
 	if len(strings.Split(value, "/")) == 1 {
@@ -2659,8 +2664,11 @@ func ValidateNamespace(namespace *api.Namespace) field.ErrorList {
 // Validate finalizer names
 func validateFinalizerName(stringValue string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if !validation.IsQualifiedName(stringValue) {
+	for _, err := range validation.IsQualifiedName(stringValue) {
-		return append(allErrs, field.Invalid(fldPath, stringValue, qualifiedNameErrorMsg))
+		allErrs = append(allErrs, field.Invalid(fldPath, stringValue, err))
+	}
+	if len(allErrs) != 0 {
+		return allErrs
 	}
 
 	if len(strings.Split(stringValue, "/")) == 1 {

pkg/api/validation/validation_test.go

@@ -310,20 +310,23 @@ func TestValidateAnnotations(t *testing.T) {
 		}
 	}
 
-	nameErrorCases := []map[string]string{
+	nameErrorCases := []struct {
-		{"nospecialchars^=@": "bar"},
+		annotations map[string]string
-		{"cantendwithadash-": "bar"},
+		expect      string
-		{"only/one/slash": "bar"},
+	}{
-		{strings.Repeat("a", 254): "bar"},
+		{map[string]string{"nospecialchars^=@": "bar"}, "must match the regex"},
+		{map[string]string{"cantendwithadash-": "bar"}, "must match the regex"},
+		{map[string]string{"only/one/slash": "bar"}, "must match the regex"},
+		{map[string]string{strings.Repeat("a", 254): "bar"}, "must be no more than"},
 	}
 	for i := range nameErrorCases {
-		errs := ValidateAnnotations(nameErrorCases[i], field.NewPath("field"))
+		errs := ValidateAnnotations(nameErrorCases[i].annotations, 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 !strings.Contains(errs[0].Detail, nameErrorCases[i].expect) {
-		if detail != qualifiedNameErrorMsg {
+				t.Errorf("case[%d]: error details do not include %q: %q", i, nameErrorCases[i].expect, errs[0].Detail)
-			t.Errorf("error detail %s should be equal %s", detail, qualifiedNameErrorMsg)
+			}
 		}
 	}
 	totalSizeErrorCases := []map[string]string{
@@ -4277,23 +4280,24 @@ func TestValidateResourceNames(t *testing.T) {
 	table := []struct {
 		input   string
 		success bool
+		expect  string
 	}{
-		{"memory", true},
+		{"memory", true, ""},
-		{"cpu", true},
+		{"cpu", true, ""},
-		{"network", false},
+		{"network", false, ""},
-		{"disk", false},
+		{"disk", false, ""},
-		{"", false},
+		{"", false, ""},
-		{".", false},
+		{".", false, ""},
-		{"..", 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, ""},
-		{"my.favorite.app.co/12345_", false},
+		{"my.favorite.app.co/12345_", false, ""},
-		{"kubernetes.io/..", false},
+		{"kubernetes.io/..", false, ""},
-		{"kubernetes.io/" + strings.Repeat("a", 63), true},
+		{"kubernetes.io/" + strings.Repeat("a", 63), true, ""},
-		{"kubernetes.io/" + strings.Repeat("a", 64), false},
+		{"kubernetes.io/" + strings.Repeat("a", 64), false, ""},
-		{"kubernetes.io//", false},
+		{"kubernetes.io//", false, ""},
-		{"kubernetes.io", false},
+		{"kubernetes.io", false, ""},
-		{"kubernetes.io/will/not/work/", false},
+		{"kubernetes.io/will/not/work/", false, ""},
 	}
 	for k, item := range table {
 		err := validateResourceName(item.input, field.NewPath("field"))
@@ -4303,8 +4307,8 @@ func TestValidateResourceNames(t *testing.T) {
 			t.Errorf("expected failure for input %q", item.input)
 			for i := range err {
 				detail := err[i].Detail
-				if detail != "" && detail != qualifiedNameErrorMsg {
+				if detail != "" && !strings.Contains(detail, item.expect) {
-					t.Errorf("%d: expected error detail either empty or %s, got %s", k, qualifiedNameErrorMsg, detail)
+					t.Errorf("%d: expected error detail either empty or %s, got %s", k, item.expect, detail)
 				}
 			}
 		}

pkg/kubelet/network/plugins.go

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

pkg/labels/selector.go

@@ -764,12 +764,11 @@ func parse(selector string) (internalSelector, error) {
 	return internalSelector(items), err
 }
 
-var qualifiedNameErrorMsg string = fmt.Sprintf(`must be a qualified name (at most %d characters, matching regex %s), with an optional DNS subdomain prefix (at most %d characters, matching regex %s) and slash (/): e.g. "MyName" or "example.com/MyName"`, validation.QualifiedNameMaxLength, validation.QualifiedNameFmt, validation.DNS1123SubdomainMaxLength, validation.DNS1123SubdomainFmt)
 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 !validation.IsQualifiedName(k) {
+	if errs := validation.IsQualifiedName(k); len(errs) != 0 {
-		return fmt.Errorf("invalid label key: %s", qualifiedNameErrorMsg)
+		return fmt.Errorf("invalid label key %q: %s", k, strings.Join(errs, "; "))
 	}
 	return nil
 }

pkg/util/validation/validation.go

@@ -17,20 +17,28 @@ limitations under the License.
 package validation
 
 import (
+	"fmt"
 	"math"
 	"net"
 	"regexp"
+	"strconv"
 	"strings"
 )
 
 const qnameCharFmt string = "[A-Za-z0-9]"
 const qnameExtCharFmt string = "[-A-Za-z0-9_.]"
-const QualifiedNameFmt string = "(" + qnameCharFmt + qnameExtCharFmt + "*)?" + qnameCharFmt
+const qualifiedNameFmt string = "(" + qnameCharFmt + qnameExtCharFmt + "*)?" + qnameCharFmt
-const QualifiedNameMaxLength int = 63
+const qualifiedNameMaxLength int = 63
 
-var qualifiedNameRegexp = regexp.MustCompile("^" + QualifiedNameFmt + "$")
+var qualifiedNameMaxLengthString = strconv.Itoa(qualifiedNameMaxLength)
+var qualifiedNameRegexp = regexp.MustCompile("^" + qualifiedNameFmt + "$")
 
-func IsQualifiedName(value string) bool {
+// IsQualifiedName tests whether the value passed is what Kubernetes calls a
+// "qualified name".  This is a format used in various places throughout the
+// system.  If the value is not valid, a list of error strings is returned.
+// Otherwise an empty list (or nil) is returned.
+func IsQualifiedName(value string) []string {
+	var errs []string
 	parts := strings.Split(value, "/")
 	var name string
 	switch len(parts) {
@@ -39,17 +47,27 @@ func IsQualifiedName(value string) bool {
 	case 2:
 		var prefix string
 		prefix, name = parts[0], parts[1]
-		if prefix == "" || !IsDNS1123Subdomain(prefix) {
+		if len(prefix) == 0 {
-			return false
+			errs = append(errs, "prefix part must be non-empty")
+		} else if !IsDNS1123Subdomain(prefix) {
+			errs = append(errs, fmt.Sprintf("prefix part must be a DNS subdomain (e.g. 'example.com')"))
 		}
 	default:
-		return false
+		return append(errs, "must match the regex "+qualifiedNameFmt+" with an optional DNS subdomain prefix and '/' (e.g. 'MyName' or 'example.com/MyName'")
 	}
 
-	return name != "" && len(name) <= QualifiedNameMaxLength && qualifiedNameRegexp.MatchString(name)
+	if len(name) == 0 {
+		errs = append(errs, "name part must be non-empty")
+	} else if len(name) > qualifiedNameMaxLength {
+		errs = append(errs, "name part must be no more than "+qualifiedNameMaxLengthString+" characters")
+	}
+	if !qualifiedNameRegexp.MatchString(name) {
+		errs = append(errs, "name part must match the regex "+qualifiedNameFmt+" (e.g. 'MyName' or 'my.name' or '123-abc')")
+	}
+	return errs
 }
 
-const LabelValueFmt string = "(" + QualifiedNameFmt + ")?"
+const LabelValueFmt string = "(" + qualifiedNameFmt + ")?"
 const LabelValueMaxLength int = 63
 
 var labelValueRegexp = regexp.MustCompile("^" + LabelValueFmt + "$")

pkg/util/validation/validation_test.go

@@ -222,8 +222,8 @@ func TestIsQualifiedName(t *testing.T) {
 		strings.Repeat("a", 253) + "/" + strings.Repeat("b", 63),
 	}
 	for i := range successCases {
-		if !IsQualifiedName(successCases[i]) {
+		if errs := IsQualifiedName(successCases[i]); len(errs) != 0 {
-			t.Errorf("case[%d]: %q: expected success", i, successCases[i])
+			t.Errorf("case[%d]: %q: expected success: %v", i, successCases[i], errs)
 		}
 	}
 
@@ -240,7 +240,7 @@ func TestIsQualifiedName(t *testing.T) {
 		strings.Repeat("a", 254) + "/abc",
 	}
 	for i := range errorCases {
-		if IsQualifiedName(errorCases[i]) {
+		if errs := IsQualifiedName(errorCases[i]); len(errs) == 0 {
 			t.Errorf("case[%d]: %q: expected failure", i, errorCases[i])
 		}
 	}

pkg/volume/plugins.go

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

plugin/pkg/scheduler/factory/factory.go

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