From 189d4a5159e8f725c09b1bed2d75c03f11561842 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 19 Dec 2015 22:52:48 -0800 Subject: [PATCH 1/6] Make CIdentifier return error strings --- pkg/api/validation/validation.go | 7 ++++--- pkg/api/validation/validation_test.go | 2 +- pkg/kubectl/run.go | 7 +++++-- pkg/util/validation/validation.go | 7 +++++-- pkg/util/validation/validation_test.go | 6 +++--- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 5b9b739f6bf..6b6b75a90b9 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -51,7 +51,6 @@ var RepairMalformedUpdates bool = true const isNegativeErrorMsg string = `must be greater than or equal to 0` const isInvalidQuotaResource string = `must be a standard resource for quota` const fieldImmutableErrorMsg string = `field is immutable` -const cIdentifierErrorMsg string = `must be a C identifier (matching regex ` + validation.CIdentifierFmt + `): e.g. "my_name" or "MyName"` const isNotIntegerErrorMsg string = `must be an integer` func InclusiveRangeErrorMsg(lo, hi int) string { @@ -1087,8 +1086,10 @@ func validateEnv(vars []api.EnvVar, fldPath *field.Path) field.ErrorList { idxPath := fldPath.Index(i) if len(ev.Name) == 0 { allErrs = append(allErrs, field.Required(idxPath.Child("name"), "")) - } else if !validation.IsCIdentifier(ev.Name) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, cIdentifierErrorMsg)) + } else { + for _, msg := range validation.IsCIdentifier(ev.Name) { + allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, msg)) + } } allErrs = append(allErrs, validateEnvVarValueFrom(ev, idxPath.Child("valueFrom"))...) } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 2781a1bdb46..25c2f931875 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -1155,7 +1155,7 @@ func TestValidateEnv(t *testing.T) { { name: "name not a C identifier", envs: []api.EnvVar{{Name: "a.b.c"}}, - expectedError: `[0].name: Invalid value: "a.b.c": must be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName"`, + expectedError: `[0].name: Invalid value: "a.b.c": must match the regex`, }, { name: "value and valueFrom specified", diff --git a/pkg/kubectl/run.go b/pkg/kubectl/run.go index 1ce9f890ef6..0b89b7814f6 100644 --- a/pkg/kubectl/run.go +++ b/pkg/kubectl/run.go @@ -835,7 +835,10 @@ func parseEnvs(envArray []string) ([]api.EnvVar, error) { } name := env[:pos] value := env[pos+1:] - if len(name) == 0 || !validation.IsCIdentifier(name) || len(value) == 0 { + if len(name) == 0 || len(value) == 0 { + return nil, fmt.Errorf("invalid env: %v", env) + } + if len(validation.IsCIdentifier(name)) != 0 { return nil, fmt.Errorf("invalid env: %v", env) } envVar := api.EnvVar{Name: name, Value: value} @@ -853,7 +856,7 @@ func parseV1Envs(envArray []string) ([]v1.EnvVar, error) { } name := env[:pos] value := env[pos+1:] - if len(name) == 0 || !validation.IsCIdentifier(name) || len(value) == 0 { + if len(name) == 0 || len(validation.IsCIdentifier(name)) != 0 || len(value) == 0 { return nil, fmt.Errorf("invalid env: %v", env) } envVar := v1.EnvVar{Name: name, Value: value} diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index 135c5e90824..bdb99b1e322 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -145,8 +145,11 @@ var cIdentifierRegexp = regexp.MustCompile("^" + CIdentifierFmt + "$") // IsCIdentifier tests for a string that conforms the definition of an identifier // in C. This checks the format, but not the length. -func IsCIdentifier(value string) bool { - return cIdentifierRegexp.MatchString(value) +func IsCIdentifier(value string) []string { + if !cIdentifierRegexp.MatchString(value) { + return []string{RegexError(CIdentifierFmt, "my_name", "MY_NAME", "MyName")} + } + return nil } // IsValidPortNum tests that the argument is a valid, non-zero port number. diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index 6af22f5b1d5..6b248351238 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -119,8 +119,8 @@ func TestIsCIdentifier(t *testing.T) { "A", "AB", "AbC", "A1", "_A", "A_", "A_B", "A_1", "A__1__2__B", "__123_ABC", } for _, val := range goodValues { - if !IsCIdentifier(val) { - t.Errorf("expected true for '%s'", val) + if msgs := IsCIdentifier(val); len(msgs) != 0 { + t.Errorf("expected true for '%s': %v", val, msgs) } } @@ -132,7 +132,7 @@ func TestIsCIdentifier(t *testing.T) { "#a#", } for _, val := range badValues { - if IsCIdentifier(val) { + if msgs := IsCIdentifier(val); len(msgs) == 0 { t.Errorf("expected false for '%s'", val) } } From 14bece550fc683966b4bf62e2d2fc893921182fc Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 4 Jan 2016 08:33:26 -0800 Subject: [PATCH 2/6] Make IsValidPortNum/Name return error strings --- pkg/api/validation/validation.go | 86 ++++++++++---------- pkg/api/validation/validation_test.go | 22 ++--- pkg/apis/extensions/validation/validation.go | 11 +-- pkg/util/validation/validation.go | 61 ++++++++------ pkg/util/validation/validation_test.go | 16 ++-- 5 files changed, 99 insertions(+), 97 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 6b6b75a90b9..eeb014497e8 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -53,14 +53,8 @@ const isInvalidQuotaResource string = `must be a standard resource for quota` const fieldImmutableErrorMsg string = `field is immutable` const isNotIntegerErrorMsg string = `must be an integer` -func InclusiveRangeErrorMsg(lo, hi int) string { - return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi) -} - -var pdPartitionErrorMsg string = InclusiveRangeErrorMsg(1, 255) -var PortRangeErrorMsg string = InclusiveRangeErrorMsg(1, 65535) -var IdRangeErrorMsg string = InclusiveRangeErrorMsg(0, math.MaxInt32) -var PortNameErrorMsg string = fmt.Sprintf(`must be an IANA_SVC_NAME (at most 15 characters, matching regex %s, it must contain at least one letter [a-z], and hyphens cannot be adjacent to other hyphens): e.g. "http"`, validation.IdentifierNoHyphensBeginEndFmt) +var pdPartitionErrorMsg string = validation.InclusiveRangeError(1, 255) +var IdRangeErrorMsg string = validation.InclusiveRangeError(0, math.MaxInt32) const totalAnnotationSizeLimitB int = 256 * (1 << 10) // 256 kB @@ -609,7 +603,7 @@ func validateISCSIVolumeSource(iscsi *api.ISCSIVolumeSource, fldPath *field.Path allErrs = append(allErrs, field.Required(fldPath.Child("iqn"), "")) } if iscsi.Lun < 0 || iscsi.Lun > 255 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), iscsi.Lun, InclusiveRangeErrorMsg(0, 255))) + allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), iscsi.Lun, validation.InclusiveRangeError(0, 255))) } return allErrs } @@ -624,7 +618,7 @@ func validateFCVolumeSource(fc *api.FCVolumeSource, fldPath *field.Path) field.E allErrs = append(allErrs, field.Required(fldPath.Child("lun"), "")) } else { if *fc.Lun < 0 || *fc.Lun > 255 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), fc.Lun, InclusiveRangeErrorMsg(0, 255))) + allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), fc.Lun, validation.InclusiveRangeError(0, 255))) } } return allErrs @@ -1054,8 +1048,10 @@ func validateContainerPorts(ports []api.ContainerPort, fldPath *field.Path) fiel for i, port := range ports { idxPath := fldPath.Index(i) if len(port.Name) > 0 { - if !validation.IsValidPortName(port.Name) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), port.Name, PortNameErrorMsg)) + if msgs := validation.IsValidPortName(port.Name); len(msgs) != 0 { + for i = range msgs { + allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), port.Name, msgs[i])) + } } else if allNames.Has(port.Name) { allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), port.Name)) } else { @@ -1063,12 +1059,16 @@ func validateContainerPorts(ports []api.ContainerPort, fldPath *field.Path) fiel } } if port.ContainerPort == 0 { - allErrs = append(allErrs, field.Invalid(idxPath.Child("containerPort"), port.ContainerPort, PortRangeErrorMsg)) - } else if !validation.IsValidPortNum(int(port.ContainerPort)) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("containerPort"), port.ContainerPort, PortRangeErrorMsg)) + allErrs = append(allErrs, field.Required(idxPath.Child("containerPort"), "")) + } else { + for _, msg := range validation.IsValidPortNum(int(port.ContainerPort)) { + allErrs = append(allErrs, field.Invalid(idxPath.Child("containerPort"), port.ContainerPort, msg)) + } } - if port.HostPort != 0 && !validation.IsValidPortNum(int(port.HostPort)) { - allErrs = append(allErrs, field.Invalid(idxPath.Child("hostPort"), port.HostPort, PortRangeErrorMsg)) + if port.HostPort != 0 { + for _, msg := range validation.IsValidPortNum(int(port.HostPort)) { + allErrs = append(allErrs, field.Invalid(idxPath.Child("hostPort"), port.HostPort, msg)) + } } if len(port.Protocol) == 0 { allErrs = append(allErrs, field.Required(idxPath.Child("protocol"), "")) @@ -1304,19 +1304,16 @@ func validateExecAction(exec *api.ExecAction, fldPath *field.Path) field.ErrorLi return allErrors } +var supportedHTTPSchemes = sets.NewString(string(api.URISchemeHTTP), string(api.URISchemeHTTPS)) + func validateHTTPGetAction(http *api.HTTPGetAction, fldPath *field.Path) field.ErrorList { allErrors := field.ErrorList{} if len(http.Path) == 0 { allErrors = append(allErrors, field.Required(fldPath.Child("path"), "")) } - if http.Port.Type == intstr.Int && !validation.IsValidPortNum(http.Port.IntValue()) { - allErrors = append(allErrors, field.Invalid(fldPath.Child("port"), http.Port, PortRangeErrorMsg)) - } else if http.Port.Type == intstr.String && !validation.IsValidPortName(http.Port.StrVal) { - allErrors = append(allErrors, field.Invalid(fldPath.Child("port"), http.Port.StrVal, PortNameErrorMsg)) - } - supportedSchemes := sets.NewString(string(api.URISchemeHTTP), string(api.URISchemeHTTPS)) - if !supportedSchemes.Has(string(http.Scheme)) { - allErrors = append(allErrors, field.Invalid(fldPath.Child("scheme"), http.Scheme, fmt.Sprintf("must be one of %v", supportedSchemes.List()))) + allErrors = append(allErrors, ValidatePortNumOrName(http.Port, fldPath.Child("port"))...) + if !supportedHTTPSchemes.Has(string(http.Scheme)) { + allErrors = append(allErrors, field.NotSupported(fldPath.Child("scheme"), http.Scheme, supportedHTTPSchemes.List())) } for _, header := range http.HTTPHeaders { if !validation.IsHTTPHeaderName(header.Name) { @@ -1326,14 +1323,24 @@ func validateHTTPGetAction(http *api.HTTPGetAction, fldPath *field.Path) field.E return allErrors } -func validateTCPSocketAction(tcp *api.TCPSocketAction, fldPath *field.Path) field.ErrorList { - allErrors := field.ErrorList{} - if tcp.Port.Type == intstr.Int && !validation.IsValidPortNum(tcp.Port.IntValue()) { - allErrors = append(allErrors, field.Invalid(fldPath.Child("port"), tcp.Port, PortRangeErrorMsg)) - } else if tcp.Port.Type == intstr.String && !validation.IsValidPortName(tcp.Port.StrVal) { - allErrors = append(allErrors, field.Invalid(fldPath.Child("port"), tcp.Port.StrVal, PortNameErrorMsg)) +func ValidatePortNumOrName(port intstr.IntOrString, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if port.Type == intstr.Int { + for _, msg := range validation.IsValidPortNum(port.IntValue()) { + allErrs = append(allErrs, field.Invalid(fldPath, port.IntValue(), msg)) + } + } else if port.Type == intstr.String { + for _, msg := range validation.IsValidPortName(port.StrVal) { + allErrs = append(allErrs, field.Invalid(fldPath, port.StrVal, msg)) + } + } else { + allErrs = append(allErrs, field.InternalError(fldPath, fmt.Errorf("unknown type: %v", port.Type))) } - return allErrors + return allErrs +} + +func validateTCPSocketAction(tcp *api.TCPSocketAction, fldPath *field.Path) field.ErrorList { + return ValidatePortNumOrName(tcp.Port, fldPath.Child("port")) } func validateHandler(handler *api.Handler, fldPath *field.Path) field.ErrorList { @@ -2162,8 +2169,8 @@ func validateServicePort(sp *api.ServicePort, requireName, isHeadlessService boo } } - if !validation.IsValidPortNum(int(sp.Port)) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("port"), sp.Port, PortRangeErrorMsg)) + for _, msg := range validation.IsValidPortNum(int(sp.Port)) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("port"), sp.Port, msg)) } if len(sp.Protocol) == 0 { @@ -2172,12 +2179,7 @@ func validateServicePort(sp *api.ServicePort, requireName, isHeadlessService boo allErrs = append(allErrs, field.NotSupported(fldPath.Child("protocol"), sp.Protocol, supportedPortProtocols.List())) } - if sp.TargetPort.Type == intstr.Int && !validation.IsValidPortNum(sp.TargetPort.IntValue()) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("targetPort"), sp.TargetPort, PortRangeErrorMsg)) - } - if sp.TargetPort.Type == intstr.String && !validation.IsValidPortName(sp.TargetPort.StrVal) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("targetPort"), sp.TargetPort, PortNameErrorMsg)) - } + allErrs = append(allErrs, ValidatePortNumOrName(sp.TargetPort, fldPath.Child("targetPort"))...) // in the v1 API, targetPorts on headless services were tolerated. // once we have version-specific validation, we can reject this on newer API versions, but until then, we have to tolerate it for compatibility. @@ -3075,8 +3077,8 @@ func validateEndpointPort(port *api.EndpointPort, requireName bool, fldPath *fie allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), port.Name, msg)) } } - if !validation.IsValidPortNum(int(port.Port)) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("port"), port.Port, PortRangeErrorMsg)) + for _, msg := range validation.IsValidPortNum(int(port.Port)) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("port"), port.Port, msg)) } if len(port.Protocol) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("protocol"), "")) diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 25c2f931875..68ae8fbcd7f 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -1030,17 +1030,17 @@ func TestValidatePorts(t *testing.T) { "name > 15 characters": { []api.ContainerPort{{Name: strings.Repeat("a", 16), ContainerPort: 80, Protocol: "TCP"}}, field.ErrorTypeInvalid, - "name", PortNameErrorMsg, + "name", "15", }, - "name not a IANA svc name ": { + "name contains invalid characters": { []api.ContainerPort{{Name: "a.b.c", ContainerPort: 80, Protocol: "TCP"}}, field.ErrorTypeInvalid, - "name", PortNameErrorMsg, + "name", "alpha-numeric", }, - "name not a IANA svc name (i.e. a number)": { + "name is a number": { []api.ContainerPort{{Name: "80", ContainerPort: 80, Protocol: "TCP"}}, field.ErrorTypeInvalid, - "name", PortNameErrorMsg, + "name", "at least one letter", }, "name not unique": { []api.ContainerPort{ @@ -1052,18 +1052,18 @@ func TestValidatePorts(t *testing.T) { }, "zero container port": { []api.ContainerPort{{ContainerPort: 0, Protocol: "TCP"}}, - field.ErrorTypeInvalid, - "containerPort", PortRangeErrorMsg, + field.ErrorTypeRequired, + "containerPort", "", }, "invalid container port": { []api.ContainerPort{{ContainerPort: 65536, Protocol: "TCP"}}, field.ErrorTypeInvalid, - "containerPort", PortRangeErrorMsg, + "containerPort", "between", }, "invalid host port": { []api.ContainerPort{{ContainerPort: 80, HostPort: 65536, Protocol: "TCP"}}, field.ErrorTypeInvalid, - "hostPort", PortRangeErrorMsg, + "hostPort", "between", }, "invalid protocol case": { []api.ContainerPort{{ContainerPort: 80, Protocol: "tcp"}}, @@ -5968,7 +5968,7 @@ func TestValidateEndpoints(t *testing.T) { }, }, errorType: "FieldValueInvalid", - errorDetail: PortRangeErrorMsg, + errorDetail: "between", }, "Invalid protocol": { endpoints: api.Endpoints{ @@ -6006,7 +6006,7 @@ func TestValidateEndpoints(t *testing.T) { }, }, errorType: "FieldValueInvalid", - errorDetail: PortRangeErrorMsg, + errorDetail: "between", }, "Port missing protocol": { endpoints: api.Endpoints{ diff --git a/pkg/apis/extensions/validation/validation.go b/pkg/apis/extensions/validation/validation.go index c061a03f391..d5e25665394 100644 --- a/pkg/apis/extensions/validation/validation.go +++ b/pkg/apis/extensions/validation/validation.go @@ -425,16 +425,7 @@ func validateIngressBackend(backend *extensions.IngressBackend, fldPath *field.P allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceName"), backend.ServiceName, msg)) } } - if backend.ServicePort.Type == intstr.String { - for _, msg := range validation.IsDNS1123Label(backend.ServicePort.StrVal) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("servicePort"), backend.ServicePort.StrVal, msg)) - } - if !validation.IsValidPortName(backend.ServicePort.StrVal) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("servicePort"), backend.ServicePort.StrVal, apivalidation.PortNameErrorMsg)) - } - } else if !validation.IsValidPortNum(backend.ServicePort.IntValue()) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("servicePort"), backend.ServicePort, apivalidation.PortRangeErrorMsg)) - } + allErrs = append(allErrs, apivalidation.ValidatePortNumOrName(backend.ServicePort, fldPath.Child("servicePort"))...) return allErrs } diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index bdb99b1e322..30a1cd16e30 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -153,8 +153,11 @@ func IsCIdentifier(value string) []string { } // IsValidPortNum tests that the argument is a valid, non-zero port number. -func IsValidPortNum(port int) bool { - return 0 < port && port < 65536 +func IsValidPortNum(port int) []string { + if port < 1 || port > 65535 { + return []string{InclusiveRangeError(1, 65535)} + } + return nil } // Now in libcontainer UID/GID limits is 0 ~ 1<<31 - 1 @@ -176,34 +179,34 @@ func IsValidUserId(uid int64) bool { return minUserID <= uid && uid <= maxUserID } -const doubleHyphensFmt string = ".*(--).*" +var portNameCharsetRegex = regexp.MustCompile("^[-a-z0-9]+$") +var portNameOneLetterRegexp = regexp.MustCompile("[a-z]") -var doubleHyphensRegexp = regexp.MustCompile("^" + doubleHyphensFmt + "$") - -const IdentifierNoHyphensBeginEndFmt string = "[a-z0-9]([a-z0-9-]*[a-z0-9])*" - -var identifierNoHyphensBeginEndRegexp = regexp.MustCompile("^" + IdentifierNoHyphensBeginEndFmt + "$") - -const atLeastOneLetterFmt string = ".*[a-z].*" - -var atLeastOneLetterRegexp = regexp.MustCompile("^" + atLeastOneLetterFmt + "$") - -// IsValidPortName check that the argument is valid syntax. It must be non empty and no more than 15 characters long -// It must contains at least one letter [a-z] and it must contains only [a-z0-9-]. -// Hypens ('-') cannot be leading or trailing character of the string and cannot be adjacent to other hyphens. -// Although RFC 6335 allows upper and lower case characters but case is ignored for comparison purposes: (HTTP -// and http denote the same service). -func IsValidPortName(port string) bool { - if len(port) < 1 || len(port) > 15 { - return false +// IsValidPortName check that the argument is valid syntax. It must be +// non-empty and no more than 15 characters long. It may contain only [-a-z0-9] +// and must contain at least one letter [a-z]. It must not start or end with a +// hyphen, nor contain adjacent hyphens. +// +// Note: We only allow lower-case characters, even though RFC 6335 is case +// insensitive. +func IsValidPortName(port string) []string { + var errs []string + if len(port) > 15 { + errs = append(errs, MaxLenError(15)) } - if doubleHyphensRegexp.MatchString(port) { - return false + if !portNameCharsetRegex.MatchString(port) { + errs = append(errs, "must contain only alpha-numeric characters (a-z, 0-9), and hyphens (-)") } - if identifierNoHyphensBeginEndRegexp.MatchString(port) && atLeastOneLetterRegexp.MatchString(port) { - return true + if !portNameOneLetterRegexp.MatchString(port) { + errs = append(errs, "must contain at least one letter (a-z)") } - return false + if strings.Contains(port, "--") { + errs = append(errs, "must not contain consecutive hyphens") + } + if len(port) > 0 && (port[0] == '-' || port[len(port)-1] == '-') { + errs = append(errs, "must not begin or end with a hyphen") + } + return errs } // IsValidIP tests that the argument is a valid IP address. @@ -263,3 +266,9 @@ func prefixEach(msgs []string, prefix string) []string { } return msgs } + +// InclusiveRangeError returns a string explanation of a numeric "must be +// between" validation failure. +func InclusiveRangeError(lo, hi int) string { + return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi) +} diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index 6b248351238..75974231e07 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -141,15 +141,15 @@ func TestIsCIdentifier(t *testing.T) { func TestIsValidPortNum(t *testing.T) { goodValues := []int{1, 2, 1000, 16384, 32768, 65535} for _, val := range goodValues { - if !IsValidPortNum(val) { - t.Errorf("expected true for '%d'", val) + if msgs := IsValidPortNum(val); len(msgs) != 0 { + t.Errorf("expected true for %d, got %v", val, msgs) } } badValues := []int{0, -1, 65536, 100000} for _, val := range badValues { - if IsValidPortNum(val) { - t.Errorf("expected false for '%d'", val) + if msgs := IsValidPortNum(val); len(msgs) == 0 { + t.Errorf("expected false for %d", val) } } } @@ -189,14 +189,14 @@ func TestIsValidUserId(t *testing.T) { func TestIsValidPortName(t *testing.T) { goodValues := []string{"telnet", "re-mail-ck", "pop3", "a", "a-1", "1-a", "a-1-b-2-c", "1-a-2-b-3"} for _, val := range goodValues { - if !IsValidPortName(val) { - t.Errorf("expected true for %q", val) + if msgs := IsValidPortName(val); len(msgs) != 0 { + t.Errorf("expected true for %q: %v", val, msgs) } } - badValues := []string{"longerthan15characters", "", "12345", "1-2-3-4", "-begin", "end-", "two--hyphens", "1-2", "whois++"} + badValues := []string{"longerthan15characters", "", strings.Repeat("a", 16), "12345", "1-2-3-4", "-begin", "end-", "two--hyphens", "whois++"} for _, val := range badValues { - if IsValidPortName(val) { + if msgs := IsValidPortName(val); len(msgs) == 0 { t.Errorf("expected false for %q", val) } } From 87c1fc50a8c0e8dab3ae9b887d8fa78b33e6e4af Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 4 Jan 2016 09:49:39 -0800 Subject: [PATCH 3/6] 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) } } From bb208a02b3ce15305d6b5ad005e418a9dce623fd Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 28 Jan 2016 23:22:57 -0800 Subject: [PATCH 4/6] Make IsValidPercent return error strings --- pkg/apis/extensions/validation/validation.go | 9 +++-- .../extensions/validation/validation_test.go | 2 +- pkg/util/validation/validation.go | 7 ++-- pkg/util/validation/validation_test.go | 36 +++++++++++++++++++ 4 files changed, 48 insertions(+), 6 deletions(-) diff --git a/pkg/apis/extensions/validation/validation.go b/pkg/apis/extensions/validation/validation.go index d5e25665394..c13d747c8d0 100644 --- a/pkg/apis/extensions/validation/validation.go +++ b/pkg/apis/extensions/validation/validation.go @@ -149,8 +149,8 @@ func ValidatePositiveIntOrPercent(intOrPercent intstr.IntOrString, fldPath *fiel allErrs := field.ErrorList{} switch intOrPercent.Type { case intstr.String: - if !validation.IsValidPercent(intOrPercent.StrVal) { - allErrs = append(allErrs, field.Invalid(fldPath, intOrPercent, "must be an integer or percentage (e.g '5%%')")) + for _, msg := range validation.IsValidPercent(intOrPercent.StrVal) { + allErrs = append(allErrs, field.Invalid(fldPath, intOrPercent, msg)) } case intstr.Int: allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(intOrPercent.IntValue()), fldPath)...) @@ -161,7 +161,10 @@ func ValidatePositiveIntOrPercent(intOrPercent intstr.IntOrString, fldPath *fiel } func getPercentValue(intOrStringValue intstr.IntOrString) (int, bool) { - if intOrStringValue.Type != intstr.String || !validation.IsValidPercent(intOrStringValue.StrVal) { + if intOrStringValue.Type != intstr.String { + return 0, false + } + if len(validation.IsValidPercent(intOrStringValue.StrVal)) != 0 { return 0, false } value, _ := strconv.Atoi(intOrStringValue.StrVal[:len(intOrStringValue.StrVal)-1]) diff --git a/pkg/apis/extensions/validation/validation_test.go b/pkg/apis/extensions/validation/validation_test.go index 8c0cc83865c..6dc72c8e355 100644 --- a/pkg/apis/extensions/validation/validation_test.go +++ b/pkg/apis/extensions/validation/validation_test.go @@ -632,7 +632,7 @@ func TestValidateDeployment(t *testing.T) { MaxSurge: intstr.FromString("20Percent"), }, } - errorCases["must be an integer or percentage"] = invalidMaxSurgeDeployment + errorCases["must match the regex"] = invalidMaxSurgeDeployment // MaxSurge and MaxUnavailable cannot both be zero. invalidRollingUpdateDeployment := validDeployment() diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index 72a132bc50a..ad7b1ff2998 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -221,8 +221,11 @@ const percentFmt string = "[0-9]+%" var percentRegexp = regexp.MustCompile("^" + percentFmt + "$") -func IsValidPercent(percent string) bool { - return percentRegexp.MatchString(percent) +func IsValidPercent(percent string) []string { + if !percentRegexp.MatchString(percent) { + return []string{RegexError(percentFmt, "1%", "93%")} + } + return nil } const HTTPHeaderNameFmt string = "[-A-Za-z0-9]+" diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index b9bcd1b7684..741df458775 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -338,3 +338,39 @@ func TestIsHTTPHeaderName(t *testing.T) { } } } + +func TestIsValidPercent(t *testing.T) { + goodValues := []string{ + "0%", + "00000%", + "1%", + "01%", + "99%", + "100%", + "101%", + } + for _, val := range goodValues { + if msgs := IsValidPercent(val); len(msgs) != 0 { + t.Errorf("expected true for %q: %v", val, msgs) + } + } + + badValues := []string{ + "", + "0", + "100", + "0.0%", + "99.9%", + "hundred", + " 1%", + "1% ", + "-0%", + "-1%", + "+1%", + } + for _, val := range badValues { + if msgs := IsValidPercent(val); len(msgs) == 0 { + t.Errorf("expected false for %q", val) + } + } +} From 3ad6c397d7487bc6b468e41fbfc7bfc30ffaaa6f Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 29 Jan 2016 00:05:34 -0800 Subject: [PATCH 5/6] Make IsValid{User,Group}Id return error strings --- pkg/api/validation/validation.go | 21 +++++++++++---------- pkg/util/validation/validation.go | 24 +++++++++++++++--------- pkg/util/validation/validation_test.go | 12 ++++++------ 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 8cfb5090537..3563f8bca26 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -19,7 +19,6 @@ package validation import ( "encoding/json" "fmt" - "math" "net" "os" "path" @@ -54,7 +53,6 @@ const fieldImmutableErrorMsg string = `field is immutable` const isNotIntegerErrorMsg string = `must be an integer` var pdPartitionErrorMsg string = validation.InclusiveRangeError(1, 255) -var IdRangeErrorMsg string = validation.InclusiveRangeError(0, math.MaxInt32) const totalAnnotationSizeLimitB int = 256 * (1 << 10) // 256 kB @@ -1889,16 +1887,19 @@ func ValidatePodSecurityContext(securityContext *api.PodSecurityContext, spec *a if securityContext != nil { allErrs = append(allErrs, validateHostNetwork(securityContext.HostNetwork, spec.Containers, specPath.Child("containers"))...) - if securityContext.FSGroup != nil && !validation.IsValidGroupId(*securityContext.FSGroup) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("fsGroup"), *(securityContext.FSGroup), IdRangeErrorMsg)) + if securityContext.FSGroup != nil { + for _, msg := range validation.IsValidGroupId(*securityContext.FSGroup) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("fsGroup"), *(securityContext.FSGroup), msg)) + } } - if securityContext.RunAsUser != nil && !validation.IsValidUserId(*securityContext.RunAsUser) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsUser"), *(securityContext.RunAsUser), IdRangeErrorMsg)) + if securityContext.RunAsUser != nil { + for _, msg := range validation.IsValidUserId(*securityContext.RunAsUser) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsUser"), *(securityContext.RunAsUser), msg)) + } } - for i, gid := range securityContext.SupplementalGroups { - if !validation.IsValidGroupId(gid) { - supplementalGroup := fmt.Sprintf(`supplementalGroups[%d]`, i) - allErrs = append(allErrs, field.Invalid(fldPath.Child(supplementalGroup), gid, IdRangeErrorMsg)) + for g, gid := range securityContext.SupplementalGroups { + for _, msg := range validation.IsValidGroupId(gid) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("supplementalGroups").Index(g), gid, msg)) } } } diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index ad7b1ff2998..75aa617ac5b 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -154,10 +154,10 @@ func IsCIdentifier(value string) []string { // IsValidPortNum tests that the argument is a valid, non-zero port number. func IsValidPortNum(port int) []string { - if port < 1 || port > 65535 { - return []string{InclusiveRangeError(1, 65535)} + if 1 <= port && port <= 65535 { + return nil } - return nil + return []string{InclusiveRangeError(1, 65535)} } // Now in libcontainer UID/GID limits is 0 ~ 1<<31 - 1 @@ -169,14 +169,20 @@ const ( maxGroupID = math.MaxInt32 ) -// IsValidGroupId tests that the argument is a valid gids. -func IsValidGroupId(gid int64) bool { - return minGroupID <= gid && gid <= maxGroupID +// IsValidGroupId tests that the argument is a valid Unix GID. +func IsValidGroupId(gid int64) []string { + if minGroupID <= gid && gid <= maxGroupID { + return nil + } + return []string{InclusiveRangeError(minGroupID, maxGroupID)} } -// IsValidUserId tests that the argument is a valid uids. -func IsValidUserId(uid int64) bool { - return minUserID <= uid && uid <= maxUserID +// IsValidUserId tests that the argument is a valid Unix UID. +func IsValidUserId(uid int64) []string { + if minUserID <= uid && uid <= maxUserID { + return nil + } + return []string{InclusiveRangeError(minUserID, maxUserID)} } var portNameCharsetRegex = regexp.MustCompile("^[-a-z0-9]+$") diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index 741df458775..47f59db41cd 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -157,14 +157,14 @@ func TestIsValidPortNum(t *testing.T) { func TestIsValidGroupId(t *testing.T) { goodValues := []int64{0, 1, 1000, 65535, 2147483647} for _, val := range goodValues { - if !IsValidGroupId(val) { - t.Errorf("expected true for '%d'", val) + if msgs := IsValidGroupId(val); len(msgs) != 0 { + t.Errorf("expected true for '%d': %v", val, msgs) } } badValues := []int64{-1, -1003, 2147483648, 4147483647} for _, val := range badValues { - if IsValidGroupId(val) { + if msgs := IsValidGroupId(val); len(msgs) == 0 { t.Errorf("expected false for '%d'", val) } } @@ -173,14 +173,14 @@ func TestIsValidGroupId(t *testing.T) { func TestIsValidUserId(t *testing.T) { goodValues := []int64{0, 1, 1000, 65535, 2147483647} for _, val := range goodValues { - if !IsValidUserId(val) { - t.Errorf("expected true for '%d'", val) + if msgs := IsValidUserId(val); len(msgs) != 0 { + t.Errorf("expected true for '%d': %v", val, msgs) } } badValues := []int64{-1, -1003, 2147483648, 4147483647} for _, val := range badValues { - if IsValidUserId(val) { + if msgs := IsValidUserId(val); len(msgs) == 0 { t.Errorf("expected false for '%d'", val) } } From 37786e0e778245234ae84666c63619f4e1dcfb86 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 14 Feb 2016 13:03:30 -0800 Subject: [PATCH 6/6] Make IsHTTPHeaderName return error strings --- pkg/api/validation/validation.go | 4 ++-- pkg/util/validation/validation.go | 11 +++++++---- pkg/util/validation/validation_test.go | 6 +++--- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 3563f8bca26..8553c82b750 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -1314,8 +1314,8 @@ func validateHTTPGetAction(http *api.HTTPGetAction, fldPath *field.Path) field.E allErrors = append(allErrors, field.NotSupported(fldPath.Child("scheme"), http.Scheme, supportedHTTPSchemes.List())) } for _, header := range http.HTTPHeaders { - if !validation.IsHTTPHeaderName(header.Name) { - allErrors = append(allErrors, field.Invalid(fldPath.Child("httpHeaders"), header.Name, fmt.Sprintf("name must match %s", validation.HTTPHeaderNameFmt))) + for _, msg := range validation.IsHTTPHeaderName(header.Name) { + allErrors = append(allErrors, field.Invalid(fldPath.Child("httpHeaders"), header.Name, msg)) } } return allErrors diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index 75aa617ac5b..ee3dba3caed 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -234,14 +234,17 @@ func IsValidPercent(percent string) []string { return nil } -const HTTPHeaderNameFmt string = "[-A-Za-z0-9]+" +const httpHeaderNameFmt string = "[-A-Za-z0-9]+" -var httpHeaderNameRegexp = regexp.MustCompile("^" + HTTPHeaderNameFmt + "$") +var httpHeaderNameRegexp = regexp.MustCompile("^" + httpHeaderNameFmt + "$") // IsHTTPHeaderName checks that a string conforms to the Go HTTP library's // definition of a valid header field name (a stricter subset than RFC7230). -func IsHTTPHeaderName(value string) bool { - return httpHeaderNameRegexp.MatchString(value) +func IsHTTPHeaderName(value string) []string { + if !httpHeaderNameRegexp.MatchString(value) { + return []string{RegexError(httpHeaderNameFmt, "X-Header-Name")} + } + return nil } // MaxLenError returns a string explanation of a "string too long" validation diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index 47f59db41cd..fe4389cddbc 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -321,8 +321,8 @@ func TestIsHTTPHeaderName(t *testing.T) { "A", "AB", "AbC", "A1", "-A", "A-", "A-B", "A-1", "A--1--2--B", "--123-ABC", } for _, val := range goodValues { - if !IsHTTPHeaderName(val) { - t.Errorf("expected true for '%s'", val) + if msgs := IsHTTPHeaderName(val); len(msgs) != 0 { + t.Errorf("expected true for '%s': %v", val, msgs) } } @@ -333,7 +333,7 @@ func TestIsHTTPHeaderName(t *testing.T) { "?", "@", "{", } for _, val := range badValues { - if IsHTTPHeaderName(val) { + if msgs := IsHTTPHeaderName(val); len(msgs) == 0 { t.Errorf("expected false for '%s'", val) } }