From 87c1fc50a8c0e8dab3ae9b887d8fa78b33e6e4af Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 4 Jan 2016 09:49:39 -0800 Subject: [PATCH] Make IsValidIP return error strings Also treat 0.0.0.0 as special, like loopback and multicast. --- pkg/api/validation/validation.go | 27 +++++++++++++++++--------- pkg/api/validation/validation_test.go | 9 ++++++++- pkg/util/validation/validation.go | 7 +++++-- pkg/util/validation/validation_test.go | 6 +++--- 4 files changed, 34 insertions(+), 15 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index eeb014497e8..8cfb5090537 100644 --- a/pkg/api/validation/validation.go +++ b/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" { - allErrs = append(allErrs, field.Invalid(idxPath, ip, "must be a valid IP address")) + if msgs := validation.IsValidIP(ip); len(msgs) != 0 { + 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) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("ip"), address.IP, "must be a valid IP address")) + for _, msg := range validation.IsValidIP(address.IP) { + 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 { - // We disallow some IPs as endpoints or external-ips. Specifically, loopback addresses are - // nonsensical and link-local addresses tend to be used for node-centric purposes (e.g. metadata service). +func validateNonSpecialIP(ipAddress string, fldPath *field.Path) field.ErrorList { + // We disallow some IPs as endpoints or external-ips. Specifically, + // 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)")) } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 68ae8fbcd7f..55c124fee70 100644 --- a/pkg/api/validation/validation_test.go +++ b/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) { diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index 30a1cd16e30..72a132bc50a 100644 --- a/pkg/util/validation/validation.go +++ b/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 { - return net.ParseIP(value) != nil +func IsValidIP(value string) []string { + 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]+%" diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index 75974231e07..b9bcd1b7684 100644 --- a/pkg/util/validation/validation_test.go +++ b/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) { - t.Errorf("expected true for %q", val) + if msgs := IsValidIP(val); len(msgs) != 0 { + 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) } }