From a5d2ca8c5510911040f30d1ed6411086fbfaddae Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Sat, 31 Mar 2018 14:13:35 -0700 Subject: [PATCH] validation: improve ProjectedVolume validation errors * only report "may not specify more than 1 volume type" once * fix incorrectly reported field paths * continue to traverse into projections to report further errors. --- pkg/apis/core/validation/validation.go | 100 +++++++++----------- pkg/apis/core/validation/validation_test.go | 65 +++++++++++++ 2 files changed, 110 insertions(+), 55 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index d41cf19567..ac05c54419 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -984,74 +984,64 @@ func validateProjectionSources(projection *core.ProjectedVolumeSource, projectio allErrs := field.ErrorList{} allPaths := sets.String{} - for _, source := range projection.Sources { + for i, source := range projection.Sources { numSources := 0 - if source.Secret != nil { - if numSources > 0 { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("secret"), "may not specify more than 1 volume type")) - } else { - numSources++ - if len(source.Secret.Name) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) - } - itemsPath := fldPath.Child("items") - for i, kp := range source.Secret.Items { - itemPath := itemsPath.Index(i) - allErrs = append(allErrs, validateKeyToPath(&kp, itemPath)...) - if len(kp.Path) > 0 { - curPath := kp.Path - if !allPaths.Has(curPath) { - allPaths.Insert(curPath) - } else { - allErrs = append(allErrs, field.Invalid(fldPath, source.Secret.Name, "conflicting duplicate paths")) - } + srcPath := fldPath.Child("sources").Index(i) + if projPath := srcPath.Child("secret"); source.Secret != nil { + numSources++ + if len(source.Secret.Name) == 0 { + allErrs = append(allErrs, field.Required(projPath.Child("name"), "")) + } + itemsPath := projPath.Child("items") + for i, kp := range source.Secret.Items { + itemPath := itemsPath.Index(i) + allErrs = append(allErrs, validateKeyToPath(&kp, itemPath)...) + if len(kp.Path) > 0 { + curPath := kp.Path + if !allPaths.Has(curPath) { + allPaths.Insert(curPath) + } else { + allErrs = append(allErrs, field.Invalid(fldPath, source.Secret.Name, "conflicting duplicate paths")) } } } } - if source.ConfigMap != nil { - if numSources > 0 { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("configMap"), "may not specify more than 1 volume type")) - } else { - numSources++ - if len(source.ConfigMap.Name) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) - } - itemsPath := fldPath.Child("items") - for i, kp := range source.ConfigMap.Items { - itemPath := itemsPath.Index(i) - allErrs = append(allErrs, validateKeyToPath(&kp, itemPath)...) - if len(kp.Path) > 0 { - curPath := kp.Path - if !allPaths.Has(curPath) { - allPaths.Insert(curPath) - } else { - allErrs = append(allErrs, field.Invalid(fldPath, source.ConfigMap.Name, "conflicting duplicate paths")) - } - + if projPath := srcPath.Child("configMap"); source.ConfigMap != nil { + numSources++ + if len(source.ConfigMap.Name) == 0 { + allErrs = append(allErrs, field.Required(projPath.Child("name"), "")) + } + itemsPath := projPath.Child("items") + for i, kp := range source.ConfigMap.Items { + itemPath := itemsPath.Index(i) + allErrs = append(allErrs, validateKeyToPath(&kp, itemPath)...) + if len(kp.Path) > 0 { + curPath := kp.Path + if !allPaths.Has(curPath) { + allPaths.Insert(curPath) + } else { + allErrs = append(allErrs, field.Invalid(fldPath, source.ConfigMap.Name, "conflicting duplicate paths")) } } } } - if source.DownwardAPI != nil { - if numSources > 0 { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("downwardAPI"), "may not specify more than 1 volume type")) - } else { - numSources++ - for _, file := range source.DownwardAPI.Items { - allErrs = append(allErrs, validateDownwardAPIVolumeFile(&file, fldPath.Child("downwardAPI"))...) - if len(file.Path) > 0 { - curPath := file.Path - if !allPaths.Has(curPath) { - allPaths.Insert(curPath) - } else { - allErrs = append(allErrs, field.Invalid(fldPath, curPath, "conflicting duplicate paths")) - } - + if projPath := srcPath.Child("downwardAPI"); source.DownwardAPI != nil { + numSources++ + for _, file := range source.DownwardAPI.Items { + allErrs = append(allErrs, validateDownwardAPIVolumeFile(&file, projPath)...) + if len(file.Path) > 0 { + curPath := file.Path + if !allPaths.Has(curPath) { + allPaths.Insert(curPath) + } else { + allErrs = append(allErrs, field.Invalid(fldPath, curPath, "conflicting duplicate paths")) } } } } + if numSources > 1 { + allErrs = append(allErrs, field.Forbidden(srcPath, "may not specify more than 1 volume type")) + } } return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 666b274f6a..416f4cd489 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -3542,6 +3542,71 @@ func TestValidateVolumes(t *testing.T) { field: "scaleIO.system", }}, }, + // ProjectedVolumeSource + { + name: "ProjectedVolumeSource more than one projection in a source", + vol: core.Volume{ + Name: "projected-volume", + VolumeSource: core.VolumeSource{ + Projected: &core.ProjectedVolumeSource{ + Sources: []core.VolumeProjection{ + { + Secret: &core.SecretProjection{ + LocalObjectReference: core.LocalObjectReference{ + Name: "foo", + }, + }, + }, + { + Secret: &core.SecretProjection{ + LocalObjectReference: core.LocalObjectReference{ + Name: "foo", + }, + }, + DownwardAPI: &core.DownwardAPIProjection{}, + }, + }, + }, + }, + }, + errs: []verr{{ + etype: field.ErrorTypeForbidden, + field: "projected.sources[1]", + }}, + }, + { + name: "ProjectedVolumeSource more than one projection in a source", + vol: core.Volume{ + Name: "projected-volume", + VolumeSource: core.VolumeSource{ + Projected: &core.ProjectedVolumeSource{ + Sources: []core.VolumeProjection{ + { + Secret: &core.SecretProjection{}, + }, + { + Secret: &core.SecretProjection{}, + DownwardAPI: &core.DownwardAPIProjection{}, + }, + }, + }, + }, + }, + errs: []verr{ + { + etype: field.ErrorTypeRequired, + field: "projected.sources[0].secret.name", + }, + { + etype: field.ErrorTypeRequired, + field: "projected.sources[1].secret.name", + }, + { + etype: field.ErrorTypeForbidden, + field: "projected.sources[1]", + }, + }, + }, } for _, tc := range testCases {