Skip to content

Commit

Permalink
audit validation errors to not double-print field names
Browse files Browse the repository at this point in the history
  • Loading branch information
@thockin
thockin committed Dec 18, 2015
1 parent cd8dfe7 commit 27fc140
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 39 deletions.
39 changes: 19 additions & 20 deletions pkg/api/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,17 @@ 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 IntervalErrorMsg(lo, hi int) string {
return fmt.Sprintf(`must be greater than %d and less than %d`, lo, hi)
func InclusiveRangeErrorMsg(lo, hi int) string {
return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi)
}

var labelValueErrorMsg string = fmt.Sprintf(`must have at most %d characters, matching regex %s: e.g. "MyValue" or ""`, validation.LabelValueMaxLength, validation.LabelValueFmt)
var qualifiedNameErrorMsg string = fmt.Sprintf(`must be a qualified name (at most %d characters, matching regex %s), with an optional DNS subdomain prefix (at most %d characters, matching regex %s) and slash (/): e.g. "MyName" or "example.com/MyName"`, validation.QualifiedNameMaxLength, validation.QualifiedNameFmt, validation.DNS1123SubdomainMaxLength, validation.DNS1123SubdomainFmt)
var DNSSubdomainErrorMsg string = fmt.Sprintf(`must be a DNS subdomain (at most %d characters, matching regex %s): e.g. "example.com"`, validation.DNS1123SubdomainMaxLength, validation.DNS1123SubdomainFmt)
var DNS1123LabelErrorMsg string = fmt.Sprintf(`must be a DNS label (at most %d characters, matching regex %s): e.g. "my-name"`, validation.DNS1123LabelMaxLength, validation.DNS1123LabelFmt)
var DNS952LabelErrorMsg string = fmt.Sprintf(`must be a DNS 952 label (at most %d characters, matching regex %s): e.g. "my-name"`, validation.DNS952LabelMaxLength, validation.DNS952LabelFmt)
var pdPartitionErrorMsg string = IntervalErrorMsg(0, 255)
var PortRangeErrorMsg string = IntervalErrorMsg(0, 65536)
var pdPartitionErrorMsg string = InclusiveRangeErrorMsg(1, 255)
var PortRangeErrorMsg string = InclusiveRangeErrorMsg(1, 65535)
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)

const totalAnnotationSizeLimitB int = 256 * (1 << 10) // 256 kB
Expand Down Expand Up @@ -276,7 +276,7 @@ func ValidateObjectMeta(meta *api.ObjectMeta, requiresNamespace bool, nameFn Val
}
} else {
if len(meta.Namespace) != 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("namespace"), meta.Namespace, "namespace is not allowed on this type"))
allErrs = append(allErrs, field.Invalid(fldPath.Child("namespace"), meta.Namespace, "not allowed on this type"))
}
}
allErrs = append(allErrs, ValidateLabels(meta.Labels, fldPath.Child("labels"))...)
Expand Down Expand Up @@ -321,7 +321,7 @@ func ValidateObjectMetaUpdate(newMeta, oldMeta *api.ObjectMeta, fldPath *field.P

// Reject updates that don't specify a resource version
if len(newMeta.ResourceVersion) == 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceVersion"), newMeta.ResourceVersion, "resourceVersion must be specified for an update"))
allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceVersion"), newMeta.ResourceVersion, "must be specified for an update"))
}

allErrs = append(allErrs, ValidateImmutableField(newMeta.Name, oldMeta.Name, fldPath.Child("name"))...)
Expand Down Expand Up @@ -464,7 +464,7 @@ func validateISCSIVolumeSource(iscsi *api.ISCSIVolumeSource, fldPath *field.Path
allErrs = append(allErrs, field.Required(fldPath.Child("fsType")))
}
if iscsi.Lun < 0 || iscsi.Lun > 255 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), iscsi.Lun, ""))
allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), iscsi.Lun, InclusiveRangeErrorMsg(0, 255)))
}
return allErrs
}
Expand All @@ -483,7 +483,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, ""))
allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), fc.Lun, InclusiveRangeErrorMsg(0, 255)))
}
}
return allErrs
Expand Down Expand Up @@ -597,11 +597,11 @@ func validateVolumeSourcePath(targetPath string, fldPath *field.Path) field.Erro

for _, item := range items {
if item == ".." {
allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not contain \"..\""))
allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not contain '..'"))
}
}
if strings.HasPrefix(items[0], "..") && len(items[0]) > 2 {
allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not start with \"..\""))
allErrs = append(allErrs, field.Invalid(fldPath, targetPath, "must not start with '..'"))
}
return allErrs
}
Expand Down Expand Up @@ -864,7 +864,7 @@ func validateObjectFieldSelector(fs *api.ObjectFieldSelector, expressions *sets.
} else {
internalFieldPath, _, err := api.Scheme.ConvertFieldLabel(fs.APIVersion, "Pod", fs.FieldPath, "")
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("fieldPath"), fs.FieldPath, "error converting fieldPath"))
allErrs = append(allErrs, field.Invalid(fldPath.Child("fieldPath"), fs.FieldPath, fmt.Sprintf("error converting fieldPath: %v", err)))
} else if !expressions.Has(internalFieldPath) {
allErrs = append(allErrs, field.NotSupported(fldPath.Child("fieldPath"), internalFieldPath, expressions.List()))
}
Expand Down Expand Up @@ -1496,7 +1496,7 @@ func ValidateReadOnlyPersistentDisks(volumes []api.Volume, fldPath *field.Path)
idxPath := fldPath.Index(i)
if vol.GCEPersistentDisk != nil {
if vol.GCEPersistentDisk.ReadOnly == false {
allErrs = append(allErrs, field.Invalid(idxPath.Child("gcePersistentDisk", ".readOnly"), false, "readOnly must be true for replicated pods > 1, as GCE PD can only be mounted on multiple machines if it is read-only."))
allErrs = append(allErrs, field.Invalid(idxPath.Child("gcePersistentDisk", "readOnly"), false, "must be true for replicated pods > 1, as GCE PD can only be mounted on multiple machines if it is read-only."))
}
}
// TODO: What to do for AWS? It doesn't support replicas
Expand Down Expand Up @@ -1526,7 +1526,7 @@ func ValidateNodeUpdate(node, oldNode *api.Node) field.ErrorList {
// TODO: Enable the code once we have better api object.status update model. Currently,
// anyone can update node status.
// if !api.Semantic.DeepEqual(node.Status, api.NodeStatus{}) {
// allErrs = append(allErrs, field.Invalid("status", node.Status, "status must be empty"))
// allErrs = append(allErrs, field.Invalid("status", node.Status, "must be empty"))
// }

// Validte no duplicate addresses in node status.
Expand Down Expand Up @@ -1800,7 +1800,7 @@ func ValidateResourceRequirements(requirements *api.ResourceRequirements, fldPat
limitValue = quantity.MilliValue()
}
if limitValue < requestValue {
allErrs = append(allErrs, field.Invalid(fldPath, quantity.String(), "limit cannot be smaller than request"))
allErrs = append(allErrs, field.Invalid(fldPath, quantity.String(), "cannot be smaller than request"))
}
}
}
Expand Down Expand Up @@ -2060,7 +2060,7 @@ func ValidateSecurityContext(sc *api.SecurityContext, fldPath *field.Path) field

if sc.RunAsUser != nil {
if *sc.RunAsUser < 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsUser"), *sc.RunAsUser, "runAsUser cannot be negative"))
allErrs = append(allErrs, field.Invalid(fldPath.Child("runAsUser"), *sc.RunAsUser, "cannot be negative"))
}
}
return allErrs
Expand All @@ -2069,18 +2069,17 @@ func ValidateSecurityContext(sc *api.SecurityContext, fldPath *field.Path) field
func ValidatePodLogOptions(opts *api.PodLogOptions) field.ErrorList {
allErrs := field.ErrorList{}
if opts.TailLines != nil && *opts.TailLines < 0 {
allErrs = append(allErrs, field.Invalid(field.NewPath("tailLines"), *opts.TailLines, "tailLines must be a non-negative integer or nil"))
allErrs = append(allErrs, field.Invalid(field.NewPath("tailLines"), *opts.TailLines, "must be a non-negative integer or nil"))
}
if opts.LimitBytes != nil && *opts.LimitBytes < 1 {
allErrs = append(allErrs, field.Invalid(field.NewPath("limitBytes"), *opts.LimitBytes, "limitBytes must be a positive integer or nil"))
allErrs = append(allErrs, field.Invalid(field.NewPath("limitBytes"), *opts.LimitBytes, "must be a positive integer or nil"))
}
switch {
case opts.SinceSeconds != nil && opts.SinceTime != nil:
allErrs = append(allErrs, field.Invalid(field.NewPath("sinceSeconds"), *opts.SinceSeconds, "only one of sinceTime or sinceSeconds can be provided"))
allErrs = append(allErrs, field.Invalid(field.NewPath("sinceTime"), *opts.SinceTime, "only one of sinceTime or sinceSeconds can be provided"))
allErrs = append(allErrs, field.Invalid(field.NewPath(""), *opts.SinceSeconds, "only one of sinceTime or sinceSeconds can be provided"))
case opts.SinceSeconds != nil:
if *opts.SinceSeconds < 1 {
allErrs = append(allErrs, field.Invalid(field.NewPath("sinceSeconds"), *opts.SinceSeconds, "sinceSeconds must be a positive integer"))
allErrs = append(allErrs, field.Invalid(field.NewPath("sinceSeconds"), *opts.SinceSeconds, "must be a positive integer"))
}
}
return allErrs
Expand Down
18 changes: 9 additions & 9 deletions pkg/api/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,17 +635,17 @@ func TestValidateVolumes(t *testing.T) {
"dot dot path": {
[]api.Volume{{Name: "dotdotpath", VolumeSource: dotDotInPath}},
field.ErrorTypeInvalid,
"downwardAPI.path", `must not contain ".."`,
"downwardAPI.path", `must not contain '..'`,
},
"dot dot file name": {
[]api.Volume{{Name: "dotdotfilename", VolumeSource: dotDotPathName}},
field.ErrorTypeInvalid,
"downwardAPI.path", `must not start with ".."`,
"downwardAPI.path", `must not start with '..'`,
},
"dot dot first level dirent": {
[]api.Volume{{Name: "dotdotdirfilename", VolumeSource: dotDotFirstLevelDirent}},
field.ErrorTypeInvalid,
"downwardAPI.path", `must not start with ".."`,
"downwardAPI.path", `must not start with '..'`,
},
"empty wwn": {
[]api.Volume{{Name: "badimage", VolumeSource: zeroWWN}},
Expand All @@ -665,12 +665,12 @@ func TestValidateVolumes(t *testing.T) {
"starts with '..'": {
[]api.Volume{{Name: "badprefix", VolumeSource: startsWithDots}},
field.ErrorTypeInvalid,
"gitRepo.directory", `must not start with ".."`,
"gitRepo.directory", `must not start with '..'`,
},
"contains '..'": {
[]api.Volume{{Name: "containsdots", VolumeSource: containsDots}},
field.ErrorTypeInvalid,
"gitRepo.directory", `must not contain ".."`,
"gitRepo.directory", `must not contain '..'`,
},
"absolute target": {
[]api.Volume{{Name: "absolutetarget", VolumeSource: absPath}},
Expand Down Expand Up @@ -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("Expected success for %s, got %v", k, errs)
t.Errorf("[%s Expected success, got %v", k, errs)
}
}

Expand All @@ -4197,12 +4197,12 @@ func TestValidateSecurityContext(t *testing.T) {
"negative RunAsUser": {
sc: negativeRunAsUser,
errorType: "FieldValueInvalid",
errorDetail: "runAsUser cannot be negative",
errorDetail: "cannot be negative",
},
}
for k, v := range errorCases {
if errs := ValidateSecurityContext(v.sc, field.NewPath("field")); len(errs) == 0 || errs[0].Type != v.errorType || errs[0].Detail != v.errorDetail {
t.Errorf("Expected error type %s with detail %s for %s, got %v", v.errorType, v.errorDetail, k, errs)
if errs := ValidateSecurityContext(v.sc, field.NewPath("field")); len(errs) == 0 || errs[0].Type != v.errorType || !strings.Contains(errs[0].Detail, v.errorDetail) {
t.Errorf("[%s] Expected error type %q with detail %q, got %v", k, v.errorType, v.errorDetail, errs)
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/apis/extensions/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ func ValidatePositiveIntOrPercent(intOrPercent intstr.IntOrString, fldPath *fiel
allErrs := field.ErrorList{}
if intOrPercent.Type == intstr.String {
if !validation.IsValidPercent(intOrPercent.StrVal) {
allErrs = append(allErrs, field.Invalid(fldPath, intOrPercent, "value should be int(5) or percentage(5%)"))
allErrs = append(allErrs, field.Invalid(fldPath, intOrPercent, "should be int(5) or percentage(5%)"))
}
} else if intOrPercent.Type == intstr.Int {
allErrs = append(allErrs, apivalidation.ValidatePositiveField(int64(intOrPercent.IntValue()), fldPath)...)
Expand Down Expand Up @@ -417,7 +417,7 @@ func ValidateIngressSpec(spec *extensions.IngressSpec, fldPath *field.Path) fiel
if spec.Backend != nil {
allErrs = append(allErrs, validateIngressBackend(spec.Backend, fldPath.Child("backend"))...)
} else if len(spec.Rules) == 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("rules"), spec.Rules, "Either a default backend or a set of host rules are required for ingress."))
allErrs = append(allErrs, field.Invalid(fldPath, spec.Rules, "either a default backend or a set of host rules are required for ingress."))
}
if len(spec.Rules) > 0 {
allErrs = append(allErrs, validateIngressRules(spec.Rules, fldPath.Child("rules"))...)
Expand Down Expand Up @@ -452,7 +452,7 @@ func validateIngressRules(IngressRules []extensions.IngressRule, fldPath *field.
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("host"), ih.Host, errMsg))
}
if isIP := (net.ParseIP(ih.Host) != nil); isIP {
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("host"), ih.Host, "Host must be a DNS name, not ip address"))
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("host"), ih.Host, "must be a DNS name, not an IP address"))
}
}
allErrs = append(allErrs, validateIngressRuleValue(&ih.IngressRuleValue, fldPath.Index(0))...)
Expand All @@ -476,7 +476,7 @@ func validateHTTPIngressRuleValue(httpIngressRuleValue *extensions.HTTPIngressRu
for i, rule := range httpIngressRuleValue.Paths {
if len(rule.Path) > 0 {
if !strings.HasPrefix(rule.Path, "/") {
allErrs = append(allErrs, field.Invalid(fldPath.Child("paths").Index(i).Child("path"), rule.Path, "must begin with /"))
allErrs = append(allErrs, field.Invalid(fldPath.Child("paths").Index(i).Child("path"), rule.Path, "must begin with '/'"))
}
// TODO: More draconian path regex validation.
// Path must be a valid regex. This is the basic requirement.
Expand All @@ -489,7 +489,7 @@ func validateHTTPIngressRuleValue(httpIngressRuleValue *extensions.HTTPIngressRu
// the user is confusing url regexes with path regexes.
_, err := regexp.CompilePOSIX(rule.Path)
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("paths").Index(i).Child("path"), rule.Path, "must be a valid regex."))
allErrs = append(allErrs, field.Invalid(fldPath.Child("paths").Index(i).Child("path"), rule.Path, "must be a valid regex"))
}
}
allErrs = append(allErrs, validateIngressBackend(&rule.Backend, fldPath.Child("backend"))...)
Expand Down Expand Up @@ -548,10 +548,10 @@ func validateClusterAutoscalerSpec(spec extensions.ClusterAutoscalerSpec, fldPat
func ValidateClusterAutoscaler(autoscaler *extensions.ClusterAutoscaler) field.ErrorList {
allErrs := field.ErrorList{}
if autoscaler.Name != "ClusterAutoscaler" {
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "name"), autoscaler.Name, `name must be ClusterAutoscaler`))
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "name"), autoscaler.Name, `must be ClusterAutoscaler`))
}
if autoscaler.Namespace != api.NamespaceDefault {
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "namespace"), autoscaler.Namespace, `namespace must be default`))
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "namespace"), autoscaler.Namespace, `must be default`))
}
allErrs = append(allErrs, validateClusterAutoscalerSpec(autoscaler.Spec, field.NewPath("spec"))...)
return allErrs
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/extensions/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,7 @@ func TestValidateDeployment(t *testing.T) {
MaxSurge: intstr.FromString("20Percent"),
},
}
errorCases["value should be int(5) or percentage(5%)"] = invalidMaxSurgeDeployment
errorCases["should be int(5) or percentage(5%)"] = invalidMaxSurgeDeployment

// MaxSurge and MaxUnavailable cannot both be zero.
invalidRollingUpdateDeployment := validDeployment()
Expand Down Expand Up @@ -1155,7 +1155,7 @@ func TestValidateClusterAutoscaler(t *testing.T) {
}

errorCases := map[string]extensions.ClusterAutoscaler{
"name must be ClusterAutoscaler": {
"must be ClusterAutoscaler": {
ObjectMeta: api.ObjectMeta{
Name: "TestClusterAutoscaler",
Namespace: api.NamespaceDefault,
Expand All @@ -1171,7 +1171,7 @@ func TestValidateClusterAutoscaler(t *testing.T) {
},
},
},
"namespace must be default": {
"must be default": {
ObjectMeta: api.ObjectMeta{
Name: "ClusterAutoscaler",
Namespace: "test",
Expand Down

0 comments on commit 27fc140

Please sign in to comment.