Make IsValidIP return error strings

Also treat 0.0.0.0 as special, like loopback and multicast.

pkg/api/validation/validation.go

@@ -2072,10 +2072,13 @@ func ValidateService(service *api.Service) field.ErrorList {
 	ipPath := specPath.Child("externalIPs")
 	for i, ip := range service.Spec.ExternalIPs {
 		idxPath := ipPath.Index(i)
-		if ip == "0.0.0.0" {
+		if msgs := validation.IsValidIP(ip); len(msgs) != 0 {
-			allErrs = append(allErrs, field.Invalid(idxPath, ip, "must be a valid IP address"))
+			for i := range msgs {
+				allErrs = append(allErrs, field.Invalid(idxPath, ip, msgs[i]))
+			}
+		} else {
+			allErrs = append(allErrs, validateNonSpecialIP(ip, idxPath)...)
 		}
-		allErrs = append(allErrs, validateIpIsNotLinkLocalOrLoopback(ip, idxPath)...)
 	}
 
 	if len(service.Spec.Type) == 0 {
@@ -3033,8 +3036,8 @@ func validateEndpointSubsets(subsets []api.EndpointSubset, fldPath *field.Path)
 
 func validateEndpointAddress(address *api.EndpointAddress, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	if !validation.IsValidIP(address.IP) {
+	for _, msg := range validation.IsValidIP(address.IP) {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("ip"), address.IP, "must be a valid IP address"))
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("ip"), address.IP, msg))
 	}
 	if len(address.Hostname) > 0 {
 		for _, msg := range validation.IsDNS1123Label(address.Hostname) {
@@ -3044,18 +3047,24 @@ func validateEndpointAddress(address *api.EndpointAddress, fldPath *field.Path)
 	if len(allErrs) > 0 {
 		return allErrs
 	}
-	return validateIpIsNotLinkLocalOrLoopback(address.IP, fldPath.Child("ip"))
+	allErrs = append(allErrs, validateNonSpecialIP(address.IP, fldPath.Child("ip"))...)
+	return allErrs
 }
 
-func validateIpIsNotLinkLocalOrLoopback(ipAddress string, fldPath *field.Path) field.ErrorList {
+func validateNonSpecialIP(ipAddress string, fldPath *field.Path) field.ErrorList {
-	// We disallow some IPs as endpoints or external-ips.  Specifically, loopback addresses are
+	// We disallow some IPs as endpoints or external-ips.  Specifically,
-	// nonsensical and link-local addresses tend to be used for node-centric purposes (e.g. metadata service).
+	// unspecified and loopback addresses are nonsensical and link-local
+	// addresses tend to be used for node-centric purposes (e.g. metadata
+	// service).
 	allErrs := field.ErrorList{}
 	ip := net.ParseIP(ipAddress)
 	if ip == nil {
 		allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "must be a valid IP address"))
 		return allErrs
 	}
+	if ip.IsUnspecified() {
+		allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be unspecified (0.0.0.0)"))
+	}
 	if ip.IsLoopback() {
 		allErrs = append(allErrs, field.Invalid(fldPath, ipAddress, "may not be in the loopback range (127.0.0.0/8)"))
 	}

pkg/api/validation/validation_test.go

@@ -3489,12 +3489,19 @@ func TestValidateService(t *testing.T) {
 			numErrs: 1,
 		},
 		{
-			name: "invalid publicIPs",
+			name: "invalid publicIPs unspecified",
 			tweakSvc: func(s *api.Service) {
 				s.Spec.ExternalIPs = []string{"0.0.0.0"}
 			},
 			numErrs: 1,
 		},
+		{
+			name: "invalid publicIPs loopback",
+			tweakSvc: func(s *api.Service) {
+				s.Spec.ExternalIPs = []string{"127.0.0.1"}
+			},
+			numErrs: 1,
+		},
 		{
 			name: "invalid publicIPs host",
 			tweakSvc: func(s *api.Service) {

pkg/util/validation/validation.go

@@ -210,8 +210,11 @@ func IsValidPortName(port string) []string {
 }
 
 // IsValidIP tests that the argument is a valid IP address.
-func IsValidIP(value string) bool {
+func IsValidIP(value string) []string {
-	return net.ParseIP(value) != nil
+	if net.ParseIP(value) == nil {
+		return []string{"must be a valid IP address, (e.g. 10.9.8.7)"}
+	}
+	return nil
 }
 
 const percentFmt string = "[0-9]+%"

pkg/util/validation/validation_test.go

@@ -293,8 +293,8 @@ func TestIsValidIP(t *testing.T) {
 		"0.0.0.0",
 	}
 	for _, val := range goodValues {
-		if !IsValidIP(val) {
+		if msgs := IsValidIP(val); len(msgs) != 0 {
-			t.Errorf("expected true for %q", val)
+			t.Errorf("expected true for %q: %v", val, msgs)
 		}
 	}
 
@@ -306,7 +306,7 @@ func TestIsValidIP(t *testing.T) {
 		"a",
 	}
 	for _, val := range badValues {
-		if IsValidIP(val) {
+		if msgs := IsValidIP(val); len(msgs) == 0 {
 			t.Errorf("expected false for %q", val)
 		}
 	}