Skip to content

K
k3s
Commit 43ed7474 authored by Tim Hockin's avatar Tim Hockin
Browse files
Options

Clean up and document validation strings

Also add a detail string for Required and Forbidden. Fix tests.
parent 27fc1400
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 87 additions and 57 deletions
docs/devel/api-conventions.md
View file @ 43ed7474
......@@ -76,6 +76,7 @@ using resources with kubectl can be found in [Working with resources](../user-gu
- [Naming conventions](#naming-conventions)
- [Label, selector, and annotation conventions](#label-selector-and-annotation-conventions)
- [WebSockets and SPDY](#websockets-and-spdy)
- [Validation](#validation)
<!-- END MUNGE: GENERATED_TOC -->
......@@ -787,6 +788,35 @@ There are two primary protocols in use today:
Clients should use the SPDY protocols if their clients have native support, or WebSockets as a fallback. Note that WebSockets is susceptible to Head-of-Line blocking and so clients must read and process each message sequentionally. In the future, an HTTP/2 implementation will be exposed that deprecates SPDY.
## Validation
API objects are validated upon receipt by the apiserver. Validation errors are
flagged and returned to the caller in a `Failure` status with `reason` set to
`Invalid`. In order to facilitate consistent error messages, we ask that
validation logic adheres to the following guidelines whenever possible (though
exceptional cases will exist).
* Be as precise as possible.
* Telling users what they CAN do is more useful than telling them what they
CANNOT do.
* When asserting a requirement in the positive, use "must". Examples: "must be
greater than 0", "must match regex '[a-z]+'". Words like "should" imply that
the assertion is optional, and must be avoided.
* When asserting a formatting requirement in the negative, use "must not".
Example: "must not contain '..'". Words like "should not" imply that the
assertion is optional, and must be avoided.
* When asserting a behavioral requirement in the negative, use "may not".
Examples: "may not be specified when otherField is empty", "only `name` may be
specified".
* When referencing a literal string value, indicate the literal in
single-quotes. Example: "must not contain '..'".
* When referencing another field name, indicate the name in back-quotes.
Example: "must be greater than `request`".
* When specifying inequalities, use words rather than symbols. Examples: "must
be less than 256", "must be greater than or equal to 0". Do not use words
like "larger than", "bigger than", "more than", "higher than", etc.
* When specifying numeric ranges, use inclusive ranges when possible.
<!-- BEGIN MUNGE: GENERATED_ANALYTICS -->
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/docs/devel/api-conventions.md?pixel)]()
<!-- END MUNGE: GENERATED_ANALYTICS -->
hack/test-cmd.sh
View file @ 43ed7474
......@@ -980,7 +980,7 @@ __EOF__
# Pre-condition: use --name flag
output_message=$(! kubectl expose -f hack/testdata/pod-with-large-name.yaml --name=invalid-large-service-name --port=8081 2>&1 "${kube_flags[@]}")
# Post-condition: should fail due to invalid name
kube::test::if_has_string "${output_message}" 'metadata.name: invalid value'
kube::test::if_has_string "${output_message}" 'metadata.name: Invalid value'
# Pre-condition: default run without --name flag; should succeed by truncating the inherited name
output_message=$(kubectl expose -f hack/testdata/pod-with-large-name.yaml --port=8081 2>&1 "${kube_flags[@]}")
# Post-condition: inherited name from pod has been truncated
......
pkg/api/errors/errors_test.go
View file @ 43ed7474
......@@ -137,7 +137,7 @@ func TestNewInvalid(t *testing.T) {
},
},
{
field.Required(field.NewPath("field[0].name")),
field.Required(field.NewPath("field[0].name"), ""),
&unversioned.StatusDetails{
Kind: "Kind",
Name: "name",
......
pkg/api/validation/validation_test.go
View file @ 43ed7474
......@@ -65,7 +65,7 @@ func TestValidateObjectMetaCustomName(t *testing.T) {
func TestValidateObjectMetaNamespaces(t *testing.T) {
errs := ValidateObjectMeta(
&api.ObjectMeta{Name: "test", Namespace: "foo.bar"},
false,
true,
func(s string, prefix bool) (bool, string) {
return true, ""
},
......@@ -84,7 +84,7 @@ func TestValidateObjectMetaNamespaces(t *testing.T) {
}
errs = ValidateObjectMeta(
&api.ObjectMeta{Name: "test", Namespace: string(b)},
false,
true,
func(s string, prefix bool) (bool, string) {
return true, ""
},
......@@ -629,7 +629,7 @@ func TestValidateVolumes(t *testing.T) {
},
"absolute path": {
[]api.Volume{{Name: "absolutepath", VolumeSource: absolutePathName}},
field.ErrorTypeForbidden,
field.ErrorTypeInvalid,
"downwardAPI.path", "",
},
"dot dot path": {
......@@ -674,7 +674,7 @@ func TestValidateVolumes(t *testing.T) {
},
"absolute target": {
[]api.Volume{{Name: "absolutetarget", VolumeSource: absPath}},
field.ErrorTypeForbidden,
field.ErrorTypeInvalid,
"gitRepo.directory", "",
},
}
......@@ -843,7 +843,7 @@ func TestValidateEnv(t *testing.T) {
},
},
}},
expectedError: "[0].valueFrom: invalid value '', Details: sources cannot be specified when value is not empty",
expectedError: "[0].valueFrom: invalid value '', Details: may not be specified when `value` is not empty",
},
{
name: "missing FieldPath on ObjectFieldSelector",
......@@ -3314,7 +3314,7 @@ func TestValidateLimitRange(t *testing.T) {
},
},
}},
"not supported when limit type is Pod",
"may not be specified when `type` is 'Pod'",
},
"default-request-limit-type-pod": {
api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
......@@ -3327,7 +3327,7 @@ func TestValidateLimitRange(t *testing.T) {
},
},
}},
"not supported when limit type is Pod",
"may not be specified when `type` is 'Pod'",
},
"min value 100m is greater than max value 10m": {
api.LimitRange{ObjectMeta: api.ObjectMeta{Name: "abc", Namespace: "foo"}, Spec: api.LimitRangeSpec{
......@@ -4002,7 +4002,7 @@ func TestValidateEndpoints(t *testing.T) {
},
},
errorType: "FieldValueInvalid",
errorDetail: "invalid IPv4 address",
errorDetail: "must be a valid IPv4 address",
},
"Multiple ports, one without name": {
endpoints: api.Endpoints{
......@@ -4052,7 +4052,7 @@ func TestValidateEndpoints(t *testing.T) {
},
},
errorType: "FieldValueInvalid",
errorDetail: "invalid IPv4 address",
errorDetail: "must be a valid IPv4 address",
},
"Port missing number": {
endpoints: api.Endpoints{
......@@ -4122,7 +4122,7 @@ func TestValidateEndpoints(t *testing.T) {
for k, v := range errorCases {
if errs := ValidateEndpoints(&v.endpoints); len(errs) == 0 || errs[0].Type != v.errorType || !strings.Contains(errs[0].Detail, v.errorDetail) {
t.Errorf("Expected error type %s with detail %s for %s, got %v", v.errorType, v.errorDetail, k, errs)
t.Errorf("[%s] Expected error type %s with detail %q, got %v", k, v.errorType, v.errorDetail, errs)
}
}
}
......@@ -4172,7 +4172,7 @@ func TestValidateSecurityContext(t *testing.T) {
}
for k, v := range successCases {
if errs := ValidateSecurityContext(v.sc, field.NewPath("field")); len(errs) != 0 {
t.Errorf("[%s Expected success, got %v", k, errs)
t.Errorf("[%s] Expected success, got %v", k, errs)
}
}
......@@ -4192,12 +4192,12 @@ func TestValidateSecurityContext(t *testing.T) {
"request privileged when capabilities forbids": {
sc: privRequestWithGlobalDeny,
errorType: "FieldValueForbidden",
errorDetail: "",
errorDetail: "disallowed by policy",
},
"negative RunAsUser": {
sc: negativeRunAsUser,
errorType: "FieldValueInvalid",
errorDetail: "cannot be negative",
errorDetail: isNegativeErrorMsg,
},
}
for k, v := range errorCases {
......
pkg/apis/extensions/validation/validation_test.go
View file @ 43ed7474
......@@ -168,7 +168,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
MaxReplicas: 5,
},
},
msg: "must be greater than or equal to 1",
msg: "must be greater than 0",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
......@@ -184,7 +184,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
MaxReplicas: 5,
},
},
msg: "must be greater than or equal to minReplicas",
msg: "must be greater than or equal to `minReplicas`",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
......@@ -201,7 +201,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: -70},
},
},
msg: "must be greater than or equal to 1",
msg: "must be greater than 0",
},
}
......@@ -746,7 +746,7 @@ func TestValidateDeployment(t *testing.T) {
invalidSelectorDeployment.Spec.Selector = map[string]string{
"name": "def",
}
errorCases["selector does not match labels"] = invalidSelectorDeployment
errorCases["`selector` does not match template `labels`"] = invalidSelectorDeployment
// RestartPolicy should be always.
invalidRestartPolicyDeployment := validDeployment()
......@@ -764,7 +764,7 @@ func TestValidateDeployment(t *testing.T) {
Type: extensions.RecreateDeploymentStrategyType,
RollingUpdate: &extensions.RollingUpdateDeployment{},
}
errorCases["should be nil when strategy type is Recreate"] = invalidRecreateDeployment
errorCases["may not be specified when strategy `type` is 'Recreate'"] = invalidRecreateDeployment
// MaxSurge should be in the form of 20%.
invalidMaxSurgeDeployment := validDeployment()
......@@ -774,7 +774,7 @@ func TestValidateDeployment(t *testing.T) {
MaxSurge: intstr.FromString("20Percent"),
},
}
errorCases["should be int(5) or percentage(5%)"] = invalidMaxSurgeDeployment
errorCases["must be an integer or percentage"] = invalidMaxSurgeDeployment
// MaxSurge and MaxUnavailable cannot both be zero.
invalidRollingUpdateDeployment := validDeployment()
......@@ -785,7 +785,7 @@ func TestValidateDeployment(t *testing.T) {
MaxUnavailable: intstr.FromInt(0),
},
}
errorCases["cannot be 0 when maxSurge is 0 as well"] = invalidRollingUpdateDeployment
errorCases["may not be 0 when `maxSurge` is 0"] = invalidRollingUpdateDeployment
// MaxUnavailable should not be more than 100%.
invalidMaxUnavailableDeployment := validDeployment()
......@@ -795,14 +795,14 @@ func TestValidateDeployment(t *testing.T) {
MaxUnavailable: intstr.FromString("110%"),
},
}
errorCases["should not be more than 100%"] = invalidMaxUnavailableDeployment
errorCases["must not be greater than 100%"] = invalidMaxUnavailableDeployment
for k, v := range errorCases {
errs := ValidateDeployment(v)
if len(errs) == 0 {
t.Errorf("expected failure for %s", k)
t.Errorf("[%s] expected failure", k)
} else if !strings.Contains(errs[0].Error(), k) {
t.Errorf("unexpected error: %v, expected: %s", errs[0], k)
t.Errorf("unexpected error: %v, expected: %q", errs[0], k)
}
}
}
......@@ -841,7 +841,7 @@ func TestValidateJob(t *testing.T) {
negative := -1
negative64 := int64(-1)
errorCases := map[string]extensions.Job{
"spec.parallelism:must be non-negative": {
"spec.parallelism:must be greater than or equal to 0": {
ObjectMeta: api.ObjectMeta{
Name: "myjob",
Namespace: api.NamespaceDefault,
......@@ -852,7 +852,7 @@ func TestValidateJob(t *testing.T) {
Template: validPodTemplateSpec,
},
},
"spec.completions:must be non-negative": {
"spec.completions:must be greater than or equal to 0": {
ObjectMeta: api.ObjectMeta{
Name: "myjob",
Namespace: api.NamespaceDefault,
......@@ -863,7 +863,7 @@ func TestValidateJob(t *testing.T) {
Template: validPodTemplateSpec,
},
},
"spec.activeDeadlineSeconds:must be non-negative": {
"spec.activeDeadlineSeconds:must be greater than or equal to 0": {
ObjectMeta: api.ObjectMeta{
Name: "myjob",
Namespace: api.NamespaceDefault,
......@@ -883,7 +883,7 @@ func TestValidateJob(t *testing.T) {
Template: validPodTemplateSpec,
},
},
"spec.template.metadata.labels: invalid value 'map[y:z]', Details: selector does not match template": {
"spec.template.metadata.labels: invalid value 'map[y:z]', Details: selector does not match template `labels`": {
ObjectMeta: api.ObjectMeta{
Name: "myjob",
Namespace: api.NamespaceDefault,
......@@ -1155,7 +1155,7 @@ func TestValidateClusterAutoscaler(t *testing.T) {
}
errorCases := map[string]extensions.ClusterAutoscaler{
"must be ClusterAutoscaler": {
"must be 'ClusterAutoscaler'": {
ObjectMeta: api.ObjectMeta{
Name: "TestClusterAutoscaler",
Namespace: api.NamespaceDefault,
......@@ -1171,7 +1171,7 @@ func TestValidateClusterAutoscaler(t *testing.T) {
},
},
},
"must be default": {
"must be 'default'": {
ObjectMeta: api.ObjectMeta{
Name: "ClusterAutoscaler",
Namespace: "test",
......@@ -1188,7 +1188,7 @@ func TestValidateClusterAutoscaler(t *testing.T) {
},
},
`must be non-negative`: {
`must be greater than or equal to 0`: {
ObjectMeta: api.ObjectMeta{
Name: "ClusterAutoscaler",
Namespace: api.NamespaceDefault,
......@@ -1204,7 +1204,7 @@ func TestValidateClusterAutoscaler(t *testing.T) {
},
},
},
`must be greater than or equal to minNodes`: {
"must be greater than or equal to `minNodes`": {
ObjectMeta: api.ObjectMeta{
Name: "ClusterAutoscaler",
Namespace: api.NamespaceDefault,
......@@ -1236,9 +1236,9 @@ func TestValidateClusterAutoscaler(t *testing.T) {
for k, v := range errorCases {
errs := ValidateClusterAutoscaler(&v)
if len(errs) == 0 {
t.Errorf("expected failure for %s", k)
t.Errorf("[%s] expected failure", k)
} else if !strings.Contains(errs[0].Error(), k) {
t.Errorf("unexpected error: %v, expected: %s", errs[0], k)
t.Errorf("unexpected error: %v, expected: %q", errs[0], k)
}
}
}
......@@ -1294,7 +1294,7 @@ func TestValidateScale(t *testing.T) {
Replicas: -1,
},
},
msg: "must be non-negative",
msg: "must be greater than or equal to 0",
},
}
......
pkg/kubectl/cmd/logs_test.go
View file @ 43ed7474
......@@ -105,17 +105,17 @@ func TestValidateLogFlags(t *testing.T) {
{
name: "since & since-time",
flags: map[string]string{"since": "1h", "since-time": "2006-01-02T15:04:05Z"},
expected: "only one of sinceTime or sinceSeconds can be provided",
expected: "at most one of `sinceTime` or `sinceSeconds` may be specified",
},
{
name: "negative limit-bytes",
flags: map[string]string{"limit-bytes": "-100"},
expected: "limitBytes must be a positive integer or nil",
expected: "must be greater than 0",
},
{
name: "negative tail",
flags: map[string]string{"tail": "-100"},
expected: "tailLines must be a non-negative integer or nil",
expected: "must be greater than or equal to 0",
},
}
for _, test := range tests {
......
pkg/util/validation/field/errors.go
View file @ 43ed7474
......@@ -47,7 +47,7 @@ func (v *Error) ErrorBody() string {
var s string
switch v.Type {
case ErrorTypeRequired, ErrorTypeTooLong, ErrorTypeInternal:
s = spew.Sprintf("%s", v.Type)
s = fmt.Sprintf("%s", v.Type)
default:
s = spew.Sprintf("%s '%+v'", v.Type, v.BadValue)
}
......@@ -65,11 +65,11 @@ type ErrorType string
// TODO: These values are duplicated in api/types.go, but there's a circular dep. Fix it.
const (
// ErrorTypeNotFound is used to report failure to find a requested value
// (e.g. looking up an ID). See NewNotFoundError.
// (e.g. looking up an ID). See NotFound().
ErrorTypeNotFound ErrorType = "FieldValueNotFound"
// ErrorTypeRequired is used to report required values that are not
// provided (e.g. empty strings, null values, or empty arrays). See
// NewRequiredError.
// Required().
ErrorTypeRequired ErrorType = "FieldValueRequired"
// ErrorTypeDuplicate is used to report collisions of values that must be
// unique (e.g. unique IDs). See Duplicate().
......@@ -90,7 +90,7 @@ const (
// too-long value. See TooLong().
ErrorTypeTooLong ErrorType = "FieldValueTooLong"
// ErrorTypeInternal is used to report other errors that are not related
// to user input.
// to user input. See InternalError().
ErrorTypeInternal ErrorType = "InternalError"
)
......@@ -119,32 +119,32 @@ func (t ErrorType) String() string {
}
}
// NewNotFoundError returns a *Error indicating "value not found". This is
// NotFound returns a *Error indicating "value not found". This is
// used to report failure to find a requested value (e.g. looking up an ID).
func NotFound(field *Path, value interface{}) *Error {
return &Error{ErrorTypeNotFound, field.String(), value, ""}
}
// NewRequiredError returns a *Error indicating "value required". This is used
// Required returns a *Error indicating "value required". This is used
// to report required values that are not provided (e.g. empty strings, null
// values, or empty arrays).
func Required(field *Path) *Error {
return &Error{ErrorTypeRequired, field.String(), "", ""}
func Required(field *Path, detail string) *Error {
return &Error{ErrorTypeRequired, field.String(), "", detail}
}
// NewDuplicateError returns a *Error indicating "duplicate value". This is
// Duplicate returns a *Error indicating "duplicate value". This is
// used to report collisions of values that must be unique (e.g. names or IDs).
func Duplicate(field *Path, value interface{}) *Error {
return &Error{ErrorTypeDuplicate, field.String(), value, ""}
}
// NewInvalidError returns a *Error indicating "invalid value". This is used
// Invalid returns a *Error indicating "invalid value". This is used
// to report malformed values (e.g. failed regex match, too long, out of bounds).
func Invalid(field *Path, value interface{}, detail string) *Error {
return &Error{ErrorTypeInvalid, field.String(), value, detail}
}
// NewNotSupportedError returns a *Error indicating "unsupported value".
// NotSupported returns a *Error indicating "unsupported value".
// This is used to report unknown values for enumerated fields (e.g. a list of
// valid values).
func NotSupported(field *Path, value interface{}, validValues []string) *Error {
......@@ -155,23 +155,23 @@ func NotSupported(field *Path, value interface{}, validValues []string) *Error {
return &Error{ErrorTypeNotSupported, field.String(), value, detail}
}
// NewForbiddenError returns a *Error indicating "forbidden". This is used to
// Forbidden returns a *Error indicating "forbidden". This is used to
// report valid (as per formatting rules) values which would be accepted under
// some conditions, but which are not permitted by current conditions (e.g.
// security policy).
func Forbidden(field *Path, value interface{}) *Error {
return &Error{ErrorTypeForbidden, field.String(), value, ""}
func Forbidden(field *Path, detail string) *Error {
return &Error{ErrorTypeForbidden, field.String(), "", detail}
}
// NewTooLongError returns a *Error indicating "too long". This is used to
// TooLong returns a *Error indicating "too long". This is used to
// report that the given value is too long. This is similar to
// NewInvalidError, but the returned error will not include the too-long
// Invalid, but the returned error will not include the too-long
// value.
func TooLong(field *Path, value interface{}, maxLength int) *Error {
return &Error{ErrorTypeTooLong, field.String(), value, fmt.Sprintf("must have at most %d characters", maxLength)}
}
// NewInternalError returns a *Error indicating "internal error". This is used
// InternalError returns a *Error indicating "internal error". This is used
// to signal that an error was found that was not directly related to user
// input. The err argument must be non-nil.
func InternalError(field *Path, err error) *Error {
......
pkg/util/validation/field/errors_test.go
View file @ 43ed7474
......@@ -44,7 +44,7 @@ func TestMakeFuncs(t *testing.T) {
ErrorTypeNotFound,
},
{
func() *Error { return Required(NewPath("f")) },
func() *Error { return Required(NewPath("f"), "d") },
ErrorTypeRequired,
},
{
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment