Clean up and document validation strings

Also add a detail string for Required and Forbidden. Fix tests.
bwreilly · Dec 18, 2015 · 43ed747 · 43ed747
1 parent 27fc140
commit 43ed747
Show file tree

Hide file tree

Showing 10 changed files with 236 additions and 199 deletions.
diff --git a/docs/devel/api-conventions.md b/docs/devel/api-conventions.md
@@ -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 -->
diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh
@@ -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

diff --git a/pkg/api/errors/errors_test.go b/pkg/api/errors/errors_test.go
@@ -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",

diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go
diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go
@@ -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 {