From 0fecf965b4844af057e5eabc6cb2e233dd64ab5c Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 14 Nov 2015 22:36:25 -0800 Subject: [PATCH] Change how one-of blocks are validated I took a hard look at error output and played until I was happier. This now prints JSON for structs in the error, rather than go's format. Also made the error message easier to read. Fixed tests. --- pkg/api/validation/validation.go | 242 +++++++++++++----- pkg/api/validation/validation_test.go | 22 +- .../extensions/validation/validation_test.go | 50 ++-- pkg/kubectl/cmd/util/helpers_test.go | 4 +- pkg/util/validation/field/errors.go | 32 ++- test/e2e/service.go | 4 +- 6 files changed, 235 insertions(+), 119 deletions(-) diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 2644a019b5c..9827e369a28 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -360,74 +360,132 @@ func validateVolumes(volumes []api.Volume, fldPath *field.Path) (sets.String, fi func validateVolumeSource(source *api.VolumeSource, fldPath *field.Path) field.ErrorList { numVolumes := 0 allErrs := field.ErrorList{} - if source.HostPath != nil { - numVolumes++ - allErrs = append(allErrs, validateHostPathVolumeSource(source.HostPath, fldPath.Child("hostPath"))...) - } if source.EmptyDir != nil { numVolumes++ // EmptyDirs have nothing to validate } + if source.HostPath != nil { + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("hostPath"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateHostPathVolumeSource(source.HostPath, fldPath.Child("hostPath"))...) + } + } if source.GitRepo != nil { - numVolumes++ - allErrs = append(allErrs, validateGitRepoVolumeSource(source.GitRepo, fldPath.Child("gitRepo"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("gitRepo"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateGitRepoVolumeSource(source.GitRepo, fldPath.Child("gitRepo"))...) + } } if source.GCEPersistentDisk != nil { - numVolumes++ - allErrs = append(allErrs, validateGCEPersistentDiskVolumeSource(source.GCEPersistentDisk, fldPath.Child("persistentDisk"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("gcePersistentDisk"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateGCEPersistentDiskVolumeSource(source.GCEPersistentDisk, fldPath.Child("persistentDisk"))...) + } } if source.AWSElasticBlockStore != nil { - numVolumes++ - allErrs = append(allErrs, validateAWSElasticBlockStoreVolumeSource(source.AWSElasticBlockStore, fldPath.Child("awsElasticBlockStore"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("awsElasticBlockStore"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateAWSElasticBlockStoreVolumeSource(source.AWSElasticBlockStore, fldPath.Child("awsElasticBlockStore"))...) + } } if source.Secret != nil { - numVolumes++ - allErrs = append(allErrs, validateSecretVolumeSource(source.Secret, fldPath.Child("secret"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("secret"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateSecretVolumeSource(source.Secret, fldPath.Child("secret"))...) + } } if source.NFS != nil { - numVolumes++ - allErrs = append(allErrs, validateNFSVolumeSource(source.NFS, fldPath.Child("nfs"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("nfs"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateNFSVolumeSource(source.NFS, fldPath.Child("nfs"))...) + } } if source.ISCSI != nil { - numVolumes++ - allErrs = append(allErrs, validateISCSIVolumeSource(source.ISCSI, fldPath.Child("iscsi"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("iscsi"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateISCSIVolumeSource(source.ISCSI, fldPath.Child("iscsi"))...) + } } if source.Glusterfs != nil { - numVolumes++ - allErrs = append(allErrs, validateGlusterfs(source.Glusterfs, fldPath.Child("glusterfs"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("glusterfs"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateGlusterfs(source.Glusterfs, fldPath.Child("glusterfs"))...) + } } if source.Flocker != nil { - numVolumes++ - allErrs = append(allErrs, validateFlockerVolumeSource(source.Flocker, fldPath.Child("flocker"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("flocker"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateFlockerVolumeSource(source.Flocker, fldPath.Child("flocker"))...) + } } if source.PersistentVolumeClaim != nil { - numVolumes++ - allErrs = append(allErrs, validatePersistentClaimVolumeSource(source.PersistentVolumeClaim, fldPath.Child("persistentVolumeClaim"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("persistentVolumeClaim"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validatePersistentClaimVolumeSource(source.PersistentVolumeClaim, fldPath.Child("persistentVolumeClaim"))...) + } } if source.RBD != nil { - numVolumes++ - allErrs = append(allErrs, validateRBDVolumeSource(source.RBD, fldPath.Child("rbd"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("rbd"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateRBDVolumeSource(source.RBD, fldPath.Child("rbd"))...) + } } if source.Cinder != nil { - numVolumes++ - allErrs = append(allErrs, validateCinderVolumeSource(source.Cinder, fldPath.Child("cinder"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("cinder"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateCinderVolumeSource(source.Cinder, fldPath.Child("cinder"))...) + } } if source.CephFS != nil { - numVolumes++ - allErrs = append(allErrs, validateCephFSVolumeSource(source.CephFS, fldPath.Child("cephfs"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("cephFS"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateCephFSVolumeSource(source.CephFS, fldPath.Child("cephfs"))...) + } } if source.DownwardAPI != nil { - numVolumes++ - allErrs = append(allErrs, validateDownwardAPIVolumeSource(source.DownwardAPI, fldPath.Child("downwardAPI"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("downwarAPI"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateDownwardAPIVolumeSource(source.DownwardAPI, fldPath.Child("downwardAPI"))...) + } } if source.FC != nil { - numVolumes++ - allErrs = append(allErrs, validateFCVolumeSource(source.FC, fldPath.Child("fc"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("fc"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateFCVolumeSource(source.FC, fldPath.Child("fc"))...) + } } if numVolumes == 0 { allErrs = append(allErrs, field.Required(fldPath, "must specify a volume type")) - } else if numVolumes > 1 { - allErrs = append(allErrs, field.Invalid(fldPath, source, "may not specify more than 1 volume type")) } return allErrs @@ -672,53 +730,95 @@ func ValidatePersistentVolume(pv *api.PersistentVolume) field.ErrorList { numVolumes := 0 if pv.Spec.HostPath != nil { - numVolumes++ - allErrs = append(allErrs, validateHostPathVolumeSource(pv.Spec.HostPath, specPath.Child("hostPath"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("hostPath"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateHostPathVolumeSource(pv.Spec.HostPath, specPath.Child("hostPath"))...) + } } if pv.Spec.GCEPersistentDisk != nil { - numVolumes++ - allErrs = append(allErrs, validateGCEPersistentDiskVolumeSource(pv.Spec.GCEPersistentDisk, specPath.Child("persistentDisk"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("gcePersistentDisk"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateGCEPersistentDiskVolumeSource(pv.Spec.GCEPersistentDisk, specPath.Child("persistentDisk"))...) + } } if pv.Spec.AWSElasticBlockStore != nil { - numVolumes++ - allErrs = append(allErrs, validateAWSElasticBlockStoreVolumeSource(pv.Spec.AWSElasticBlockStore, specPath.Child("awsElasticBlockStore"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("awsElasticBlockStore"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateAWSElasticBlockStoreVolumeSource(pv.Spec.AWSElasticBlockStore, specPath.Child("awsElasticBlockStore"))...) + } } if pv.Spec.Glusterfs != nil { - numVolumes++ - allErrs = append(allErrs, validateGlusterfs(pv.Spec.Glusterfs, specPath.Child("glusterfs"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("glusterfs"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateGlusterfs(pv.Spec.Glusterfs, specPath.Child("glusterfs"))...) + } } if pv.Spec.Flocker != nil { - numVolumes++ - allErrs = append(allErrs, validateFlockerVolumeSource(pv.Spec.Flocker, specPath.Child("flocker"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("flocker"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateFlockerVolumeSource(pv.Spec.Flocker, specPath.Child("flocker"))...) + } } if pv.Spec.NFS != nil { - numVolumes++ - allErrs = append(allErrs, validateNFSVolumeSource(pv.Spec.NFS, specPath.Child("nfs"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("nfs"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateNFSVolumeSource(pv.Spec.NFS, specPath.Child("nfs"))...) + } } if pv.Spec.RBD != nil { - numVolumes++ - allErrs = append(allErrs, validateRBDVolumeSource(pv.Spec.RBD, specPath.Child("rbd"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("rbd"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateRBDVolumeSource(pv.Spec.RBD, specPath.Child("rbd"))...) + } } if pv.Spec.CephFS != nil { - numVolumes++ - allErrs = append(allErrs, validateCephFSVolumeSource(pv.Spec.CephFS, specPath.Child("cephfs"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("cephFS"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateCephFSVolumeSource(pv.Spec.CephFS, specPath.Child("cephfs"))...) + } } if pv.Spec.ISCSI != nil { - numVolumes++ - allErrs = append(allErrs, validateISCSIVolumeSource(pv.Spec.ISCSI, specPath.Child("iscsi"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("iscsi"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateISCSIVolumeSource(pv.Spec.ISCSI, specPath.Child("iscsi"))...) + } } if pv.Spec.Cinder != nil { - numVolumes++ - allErrs = append(allErrs, validateCinderVolumeSource(pv.Spec.Cinder, specPath.Child("cinder"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("cinder"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateCinderVolumeSource(pv.Spec.Cinder, specPath.Child("cinder"))...) + } } if pv.Spec.FC != nil { - numVolumes++ - allErrs = append(allErrs, validateFCVolumeSource(pv.Spec.FC, specPath.Child("fc"))...) + if numVolumes > 0 { + allErrs = append(allErrs, field.Forbidden(specPath.Child("fc"), "may not specify more than 1 volume type")) + } else { + numVolumes++ + allErrs = append(allErrs, validateFCVolumeSource(pv.Spec.FC, specPath.Child("fc"))...) + } } if numVolumes == 0 { allErrs = append(allErrs, field.Required(specPath, "must specify a volume type")) - } else if numVolumes > 1 { - allErrs = append(allErrs, field.Invalid(specPath, pv.Spec.PersistentVolumeSource, "may not specify more than 1 volume type")) } return allErrs } @@ -979,21 +1079,31 @@ func validateHandler(handler *api.Handler, fldPath *field.Path) field.ErrorList numHandlers := 0 allErrors := field.ErrorList{} if handler.Exec != nil { - numHandlers++ - allErrors = append(allErrors, validateExecAction(handler.Exec, fldPath.Child("exec"))...) + if numHandlers > 0 { + allErrors = append(allErrors, field.Forbidden(fldPath.Child("exec"), "may not specify more than 1 handler type")) + } else { + numHandlers++ + allErrors = append(allErrors, validateExecAction(handler.Exec, fldPath.Child("exec"))...) + } } if handler.HTTPGet != nil { - numHandlers++ - allErrors = append(allErrors, validateHTTPGetAction(handler.HTTPGet, fldPath.Child("httpGet"))...) + if numHandlers > 0 { + allErrors = append(allErrors, field.Forbidden(fldPath.Child("httpGet"), "may not specify more than 1 handler type")) + } else { + numHandlers++ + allErrors = append(allErrors, validateHTTPGetAction(handler.HTTPGet, fldPath.Child("httpGet"))...) + } } if handler.TCPSocket != nil { - numHandlers++ - allErrors = append(allErrors, validateTCPSocketAction(handler.TCPSocket, fldPath.Child("tcpSocket"))...) + if numHandlers > 0 { + allErrors = append(allErrors, field.Forbidden(fldPath.Child("tcpSocket"), "may not specify more than 1 handler type")) + } else { + numHandlers++ + allErrors = append(allErrors, validateTCPSocketAction(handler.TCPSocket, fldPath.Child("tcpSocket"))...) + } } if numHandlers == 0 { allErrors = append(allErrors, field.Required(fldPath, "must specify a handler type")) - } else if numHandlers > 1 { - allErrors = append(allErrors, field.Invalid(fldPath, handler, "may not specify more than 1 handler type")) } return allErrors } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index c22b05478c4..d7e846b64c9 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -73,7 +73,7 @@ func TestValidateObjectMetaNamespaces(t *testing.T) { if len(errs) != 1 { t.Fatalf("unexpected errors: %v", errs) } - if !strings.Contains(errs[0].Error(), "invalid value 'foo.bar'") { + if !strings.Contains(errs[0].Error(), `Invalid value: "foo.bar"`) { t.Errorf("unexpected error message: %v", errs) } maxLength := 63 @@ -92,7 +92,7 @@ func TestValidateObjectMetaNamespaces(t *testing.T) { if len(errs) != 1 { t.Fatalf("unexpected errors: %v", errs) } - if !strings.Contains(errs[0].Error(), "invalid value") { + if !strings.Contains(errs[0].Error(), "Invalid value") { t.Errorf("unexpected error message: %v", errs) } } @@ -824,12 +824,12 @@ func TestValidateEnv(t *testing.T) { { name: "zero-length name", envs: []api.EnvVar{{Name: ""}}, - expectedError: "[0].name: required value", + expectedError: "[0].name: Required value", }, { name: "name not a C identifier", envs: []api.EnvVar{{Name: "a.b.c"}}, - expectedError: `[0].name: invalid value 'a.b.c', Details: 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 be a C identifier (matching regex [A-Za-z_][A-Za-z0-9_]*): e.g. "my_name" or "MyName"`, }, { name: "value and valueFrom specified", @@ -843,7 +843,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom: invalid value '', Details: may not be specified when `value` is not empty", + expectedError: "[0].valueFrom: Invalid value: \"\": may not be specified when `value` is not empty", }, { name: "missing FieldPath on ObjectFieldSelector", @@ -855,7 +855,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom.fieldRef.fieldPath: required value", + expectedError: `[0].valueFrom.fieldRef.fieldPath: Required value`, }, { name: "missing APIVersion on ObjectFieldSelector", @@ -867,7 +867,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom.fieldRef.apiVersion: required value", + expectedError: `[0].valueFrom.fieldRef.apiVersion: Required value`, }, { name: "invalid fieldPath", @@ -880,7 +880,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom.fieldRef.fieldPath: invalid value 'metadata.whoops', Details: error converting fieldPath", + expectedError: `[0].valueFrom.fieldRef.fieldPath: Invalid value: "metadata.whoops": error converting fieldPath`, }, { name: "invalid fieldPath labels", @@ -893,7 +893,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom.fieldRef.fieldPath: unsupported value 'metadata.labels', Details: supported values: metadata.name, metadata.namespace, status.podIP", + expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.labels": supported values: metadata.name, metadata.namespace, status.podIP`, }, { name: "invalid fieldPath annotations", @@ -906,7 +906,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "[0].valueFrom.fieldRef.fieldPath: unsupported value 'metadata.annotations', Details: supported values: metadata.name, metadata.namespace, status.podIP", + expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.annotations": supported values: metadata.name, metadata.namespace, status.podIP`, }, { name: "unsupported fieldPath", @@ -919,7 +919,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: "valueFrom.fieldRef.fieldPath: unsupported value 'status.phase', Details: supported values: metadata.name, metadata.namespace, status.podIP", + expectedError: `valueFrom.fieldRef.fieldPath: Unsupported value: "status.phase": supported values: metadata.name, metadata.namespace, status.podIP`, }, } for _, tc := range errorCases { diff --git a/pkg/apis/extensions/validation/validation_test.go b/pkg/apis/extensions/validation/validation_test.go index d29e54486b0..62f61cde021 100644 --- a/pkg/apis/extensions/validation/validation_test.go +++ b/pkg/apis/extensions/validation/validation_test.go @@ -80,7 +80,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70}, }, }, - msg: "scaleRef.kind: required", + msg: "scaleRef.kind: Required", }, { horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{ @@ -92,7 +92,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70}, }, }, - msg: "scaleRef.kind: invalid", + msg: "scaleRef.kind: Invalid", }, { horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{ @@ -104,7 +104,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70}, }, }, - msg: "scaleRef.name: required", + msg: "scaleRef.name: Required", }, { horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{ @@ -116,7 +116,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70}, }, }, - msg: "scaleRef.name: invalid", + msg: "scaleRef.name: Invalid", }, { horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{ @@ -128,7 +128,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70}, }, }, - msg: "scaleRef.subresource: required", + msg: "scaleRef.subresource: Required", }, { horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{ @@ -140,7 +140,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70}, }, }, - msg: "scaleRef.subresource: invalid", + msg: "scaleRef.subresource: Invalid", }, { horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{ @@ -152,7 +152,7 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) { CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70}, }, }, - msg: "scaleRef.subresource: unsupported", + msg: "scaleRef.subresource: Unsupported", }, { horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{ @@ -736,7 +736,7 @@ func TestValidateDeployment(t *testing.T) { } errorCases := map[string]*extensions.Deployment{} - errorCases["metadata.name: required value"] = &extensions.Deployment{ + errorCases["metadata.name: Required value"] = &extensions.Deployment{ ObjectMeta: api.ObjectMeta{ Namespace: api.NamespaceDefault, }, @@ -751,12 +751,12 @@ func TestValidateDeployment(t *testing.T) { // RestartPolicy should be always. invalidRestartPolicyDeployment := validDeployment() invalidRestartPolicyDeployment.Spec.Template.Spec.RestartPolicy = api.RestartPolicyNever - errorCases["unsupported value 'Never'"] = invalidRestartPolicyDeployment + errorCases["Unsupported value: \"Never\""] = invalidRestartPolicyDeployment // invalid unique label key. invalidUniqueLabelDeployment := validDeployment() invalidUniqueLabelDeployment.Spec.UniqueLabelKey = "abc/def/ghi" - errorCases["spec.uniqueLabel: invalid value"] = invalidUniqueLabelDeployment + errorCases["spec.uniqueLabel: Invalid value"] = invalidUniqueLabelDeployment // rollingUpdate should be nil for recreate. invalidRecreateDeployment := validDeployment() @@ -802,7 +802,7 @@ func TestValidateDeployment(t *testing.T) { if len(errs) == 0 { t.Errorf("[%s] expected failure", k) } else if !strings.Contains(errs[0].Error(), k) { - t.Errorf("unexpected error: %v, expected: %q", errs[0], k) + t.Errorf("unexpected error: %q, expected: %q", errs[0].Error(), k) } } } @@ -874,7 +874,7 @@ func TestValidateJob(t *testing.T) { Template: validPodTemplateSpec, }, }, - "spec.selector:required value": { + "spec.selector:Required value": { 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 `labels`": { + "spec.template.metadata.labels: Invalid value: {\"y\":\"z\"}: `selector` does not match template `labels`": { ObjectMeta: api.ObjectMeta{ Name: "myjob", Namespace: api.NamespaceDefault, @@ -902,7 +902,7 @@ func TestValidateJob(t *testing.T) { }, }, }, - "spec.template.spec.restartPolicy:unsupported value": { + "spec.template.spec.restartPolicy: Unsupported value": { ObjectMeta: api.ObjectMeta{ Name: "myjob", Namespace: api.NamespaceDefault, @@ -1006,19 +1006,19 @@ func TestValidateIngress(t *testing.T) { Backend: defaultBackend, }, } - badPathErr := fmt.Sprintf("spec.rules[0].http.paths[0].path: invalid value '%v'", badPathExpr) + badPathErr := fmt.Sprintf("spec.rules[0].http.paths[0].path: Invalid value: '%v'", badPathExpr) hostIP := "127.0.0.1" badHostIP := newValid() badHostIP.Spec.Rules[0].Host = hostIP - badHostIPErr := fmt.Sprintf("spec.rules[0].host: invalid value '%v'", hostIP) + badHostIPErr := fmt.Sprintf("spec.rules[0].host: Invalid value: '%v'", hostIP) errorCases := map[string]extensions.Ingress{ - "spec.backend.serviceName: required value": servicelessBackend, - "spec.backend.serviceName: invalid value": invalidNameBackend, - "spec.backend.servicePort: invalid value": noPortBackend, - "spec.rules[0].host: invalid value": badHost, - "spec.rules[0].http.paths: required value": noPaths, - "spec.rules[0].http.paths[0].path: invalid value": noForwardSlashPath, + "spec.backend.serviceName: Required value": servicelessBackend, + "spec.backend.serviceName: Invalid value": invalidNameBackend, + "spec.backend.servicePort: Invalid value": noPortBackend, + "spec.rules[0].host: Invalid value": badHost, + "spec.rules[0].http.paths: Required value": noPaths, + "spec.rules[0].http.paths[0].path: Invalid value": noForwardSlashPath, } errorCases[badPathErr] = badRegexPath errorCases[badHostIPErr] = badHostIP @@ -1112,8 +1112,8 @@ func TestValidateIngressStatusUpdate(t *testing.T) { } errorCases := map[string]extensions.Ingress{ - "status.loadBalancer.ingress[0].ip: invalid value": invalidIP, - "status.loadBalancer.ingress[0].hostname: invalid value": invalidHostname, + "status.loadBalancer.ingress[0].ip: Invalid value": invalidIP, + "status.loadBalancer.ingress[0].hostname: Invalid value": invalidHostname, } for k, v := range errorCases { errs := ValidateIngressStatusUpdate(&v, &oldValue) @@ -1220,7 +1220,7 @@ func TestValidateClusterAutoscaler(t *testing.T) { }, }, }, - "required value": { + "Required value": { ObjectMeta: api.ObjectMeta{ Name: "ClusterAutoscaler", Namespace: api.NamespaceDefault, diff --git a/pkg/kubectl/cmd/util/helpers_test.go b/pkg/kubectl/cmd/util/helpers_test.go index 574012a2f63..20191a5e4ec 100644 --- a/pkg/kubectl/cmd/util/helpers_test.go +++ b/pkg/kubectl/cmd/util/helpers_test.go @@ -275,11 +275,11 @@ func TestCheckInvalidErr(t *testing.T) { }{ { errors.NewInvalid(api.Kind("Invalid1"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field"), "single", "details")}), - `Error from server: Invalid1 "invalidation" is invalid: field: invalid value 'single', Details: details`, + `Error from server: Invalid1 "invalidation" is invalid: field: Invalid value: "single": details`, }, { errors.NewInvalid(api.Kind("Invalid2"), "invalidation", field.ErrorList{field.Invalid(field.NewPath("field1"), "multi1", "details"), field.Invalid(field.NewPath("field2"), "multi2", "details")}), - `Error from server: Invalid2 "invalidation" is invalid: [field1: invalid value 'multi1', Details: details, field2: invalid value 'multi2', Details: details]`, + `Error from server: Invalid2 "invalidation" is invalid: [field1: Invalid value: "multi1": details, field2: Invalid value: "multi2": details]`, }, { errors.NewInvalid(api.Kind("Invalid3"), "invalidation", field.ErrorList{}), diff --git a/pkg/util/validation/field/errors.go b/pkg/util/validation/field/errors.go index 4d0cc4aa036..0a8ed1e8565 100644 --- a/pkg/util/validation/field/errors.go +++ b/pkg/util/validation/field/errors.go @@ -17,12 +17,11 @@ limitations under the License. package field import ( + "encoding/json" "fmt" "strings" utilerrors "k8s.io/kubernetes/pkg/util/errors" - - "github.com/davecgh/go-spew/spew" ) // Error is an implementation of the 'error' interface, which represents a @@ -46,13 +45,20 @@ func (v *Error) Error() string { func (v *Error) ErrorBody() string { var s string switch v.Type { - case ErrorTypeRequired, ErrorTypeTooLong, ErrorTypeInternal: + case ErrorTypeRequired, ErrorTypeForbidden, ErrorTypeTooLong, ErrorTypeInternal: s = fmt.Sprintf("%s", v.Type) default: - s = spew.Sprintf("%s '%+v'", v.Type, v.BadValue) + var bad string + badBytes, err := json.Marshal(v.BadValue) + if err != nil { + bad = err.Error() + } else { + bad = string(badBytes) + } + s = fmt.Sprintf("%s: %s", v.Type, bad) } if len(v.Detail) != 0 { - s += fmt.Sprintf(", Details: %s", v.Detail) + s += fmt.Sprintf(": %s", v.Detail) } return s } @@ -98,21 +104,21 @@ const ( func (t ErrorType) String() string { switch t { case ErrorTypeNotFound: - return "not found" + return "Not found" case ErrorTypeRequired: - return "required value" + return "Required value" case ErrorTypeDuplicate: - return "duplicate value" + return "Duplicate value" case ErrorTypeInvalid: - return "invalid value" + return "Invalid value" case ErrorTypeNotSupported: - return "unsupported value" + return "Unsupported value" case ErrorTypeForbidden: - return "forbidden" + return "Forbidden" case ErrorTypeTooLong: - return "too long" + return "Too long" case ErrorTypeInternal: - return "internal error" + return "Internal error" default: panic(fmt.Sprintf("unrecognized validation error: %q", t)) return "" diff --git a/test/e2e/service.go b/test/e2e/service.go index fc952308e80..bd8c25dbf74 100644 --- a/test/e2e/service.go +++ b/test/e2e/service.go @@ -643,7 +643,7 @@ var _ = Describe("Services", func() { if err == nil { Failf("Created service with conflicting NodePort: %v", result2) } - expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: invalid value '%d', Details: provided port is already allocated", + expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: Invalid value: %d: provided port is already allocated", serviceName2, port.NodePort) Expect(fmt.Sprintf("%v", err)).To(Equal(expectedErr)) @@ -704,7 +704,7 @@ var _ = Describe("Services", func() { if err == nil { Failf("failed to prevent update of service with out-of-range NodePort: %v", result) } - expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: invalid value '%d', Details: provided port is not in the valid range", serviceName, outOfRangeNodePort) + expectedErr := fmt.Sprintf("Service \"%s\" is invalid: spec.ports[0].nodePort: Invalid value: %d: provided port is not in the valid range", serviceName, outOfRangeNodePort) Expect(fmt.Sprintf("%v", err)).To(Equal(expectedErr)) By("deleting original service " + serviceName)