From 98e2c8cdee62e7cb804e7dc429083cfede8ef3ee Mon Sep 17 00:00:00 2001 From: Ian Chakeres Date: Sat, 11 Nov 2017 15:39:37 -0800 Subject: [PATCH] Validate that PV capacity and PVC capacity requests are greater than zero --- pkg/apis/core/validation/validation.go | 16 ++++++-- pkg/apis/core/validation/validation_test.go | 38 +++++++++++++++++++ .../volume/persistent_volumes_test.go | 2 +- 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index b2b82c17f70..962fd49cd86 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -62,7 +62,7 @@ const isNegativeErrorMsg string = apimachineryvalidation.IsNegativeErrorMsg const isInvalidQuotaResource string = `must be a standard resource for quota` const fieldImmutableErrorMsg string = genericvalidation.FieldImmutableErrorMsg const isNotIntegerErrorMsg string = `must be an integer` -const isZeroErrorMsg string = `must be greater than zero` +const isNotPositiveErrorMsg string = `must be greater than zero` var pdPartitionErrorMsg string = validation.InclusiveRangeError(1, 255) var volumeModeErrorMsg string = "must be a number between 0 and 0777 (octal), both inclusive" @@ -315,6 +315,15 @@ func ValidateNonnegativeQuantity(value resource.Quantity, fldPath *field.Path) f return allErrs } +// Validates that a Quantity is positive +func ValidatePositiveQuantityValue(value resource.Quantity, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if value.Cmp(resource.Quantity{}) <= 0 { + allErrs = append(allErrs, field.Invalid(fldPath, value.String(), isNotPositiveErrorMsg)) + } + return allErrs +} + func ValidateImmutableField(newVal, oldVal interface{}, fldPath *field.Path) field.ErrorList { return genericvalidation.ValidateImmutableField(newVal, oldVal, fldPath) } @@ -1342,6 +1351,7 @@ func ValidatePersistentVolume(pv *core.PersistentVolume) field.ErrorList { capPath := specPath.Child("capacity") for r, qty := range pv.Spec.Capacity { allErrs = append(allErrs, validateBasicResource(qty, capPath.Key(string(r)))...) + allErrs = append(allErrs, ValidatePositiveQuantityValue(qty, capPath.Key(string(r)))...) } if len(string(pv.Spec.PersistentVolumeReclaimPolicy)) > 0 { if !supportedReclaimPolicy.Has(string(pv.Spec.PersistentVolumeReclaimPolicy)) { @@ -1602,9 +1612,7 @@ func ValidatePersistentVolumeClaimSpec(spec *core.PersistentVolumeClaimSpec, fld allErrs = append(allErrs, field.Required(fldPath.Child("resources").Key(string(core.ResourceStorage)), "")) } else { allErrs = append(allErrs, ValidateResourceQuantityValue(string(core.ResourceStorage), storageValue, fldPath.Child("resources").Key(string(core.ResourceStorage)))...) - if storageValue.Value() == int64(0) { - allErrs = append(allErrs, field.Invalid(fldPath, storageValue, isZeroErrorMsg)) - } + allErrs = append(allErrs, ValidatePositiveQuantityValue(storageValue, fldPath.Child("resources").Key(string(core.ResourceStorage)))...) } if spec.StorageClassName != nil && len(*spec.StorageClassName) > 0 { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 403d230d588..816c653adf2 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -260,6 +260,21 @@ func TestValidatePersistentVolumes(t *testing.T) { }, }), }, + "bad-volume-zero-capacity": { + isExpectedFailure: true, + volume: testVolume("foo", "", core.PersistentVolumeSpec{ + Capacity: core.ResourceList{ + core.ResourceName(core.ResourceStorage): resource.MustParse("0"), + }, + AccessModes: []core.PersistentVolumeAccessMode{core.ReadWriteOnce}, + PersistentVolumeSource: core.PersistentVolumeSource{ + HostPath: &core.HostPathVolumeSource{ + Path: "/foo", + Type: newHostPathType(string(core.HostPathDirectory)), + }, + }, + }), + }, "missing-accessmodes": { isExpectedFailure: true, volume: testVolume("goodname", "missing-accessmodes", core.PersistentVolumeSpec{ @@ -732,6 +747,29 @@ func TestValidatePersistentVolumeClaim(t *testing.T) { StorageClassName: &validClassName, }), }, + "invalid-claim-zero-capacity": { + isExpectedFailure: true, + claim: testVolumeClaim("foo", "ns", core.PersistentVolumeClaimSpec{ + Selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key2", + Operator: "Exists", + }, + }, + }, + AccessModes: []core.PersistentVolumeAccessMode{ + core.ReadWriteOnce, + core.ReadOnlyMany, + }, + Resources: core.ResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceStorage): resource.MustParse("0G"), + }, + }, + StorageClassName: &validClassName, + }), + }, "invalid-label-selector": { isExpectedFailure: true, claim: testVolumeClaim("foo", "ns", core.PersistentVolumeClaimSpec{ diff --git a/test/integration/volume/persistent_volumes_test.go b/test/integration/volume/persistent_volumes_test.go index 2a1860a67d3..aaa409d3365 100644 --- a/test/integration/volume/persistent_volumes_test.go +++ b/test/integration/volume/persistent_volumes_test.go @@ -494,7 +494,7 @@ func TestPersistentVolumeMultiPVs(t *testing.T) { pvs := make([]*v1.PersistentVolume, maxPVs) for i := 0; i < maxPVs; i++ { // This PV will be claimed, released, and deleted - pvs[i] = createPV("pv-"+strconv.Itoa(i), "/tmp/foo"+strconv.Itoa(i), strconv.Itoa(i)+"G", + pvs[i] = createPV("pv-"+strconv.Itoa(i), "/tmp/foo"+strconv.Itoa(i), strconv.Itoa(i+1)+"G", []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}, v1.PersistentVolumeReclaimRetain) }