From 5fcffcf4e48ff450033d4fe2a5e54376579098af Mon Sep 17 00:00:00 2001 From: carlory Date: Wed, 5 Jul 2023 15:31:15 +0800 Subject: [PATCH] Add APIGroup ratcheting validation to PVC.DataSource --- pkg/apis/core/validation/validation.go | 35 +++++- pkg/apis/core/validation/validation_test.go | 113 ++++++++++++++++++++ 2 files changed, 144 insertions(+), 4 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index d66823eeb85..ecf849b4e33 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -2022,6 +2022,8 @@ type PersistentVolumeClaimSpecValidationOptions struct { EnableRecoverFromExpansionFailure bool // Allow to validate the label value of the label selector AllowInvalidLabelValueInSelector bool + // Allow to validate the API group of the data source and data source reference + AllowInvalidAPIGroupInDataSourceOrRef bool } func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolumeClaim) PersistentVolumeClaimSpecValidationOptions { @@ -2034,6 +2036,10 @@ func ValidationOptionsForPersistentVolumeClaim(pvc, oldPvc *core.PersistentVolum // If there's no old PVC, use the options based solely on feature enablement return opts } + + // If the old object had an invalid API group in the data source or data source reference, continue to allow it in the new object + opts.AllowInvalidAPIGroupInDataSourceOrRef = allowInvalidAPIGroupInDataSourceOrRef(&oldPvc.Spec) + labelSelectorValidationOpts := unversionedvalidation.LabelSelectorValidationOptions{ AllowInvalidLabelValueInSelector: opts.AllowInvalidLabelValueInSelector, } @@ -2077,6 +2083,17 @@ func ValidationOptionsForPersistentVolumeClaimTemplate(claimTemplate, oldClaimTe return opts } +// allowInvalidAPIGroupInDataSourceOrRef returns true if the spec contains a data source or data source reference with an API group +func allowInvalidAPIGroupInDataSourceOrRef(spec *core.PersistentVolumeClaimSpec) bool { + if spec.DataSource != nil && spec.DataSource.APIGroup != nil { + return true + } + if spec.DataSourceRef != nil && spec.DataSourceRef.APIGroup != nil { + return true + } + return false +} + // ValidatePersistentVolumeClaim validates a PersistentVolumeClaim func ValidatePersistentVolumeClaim(pvc *core.PersistentVolumeClaim, opts PersistentVolumeClaimSpecValidationOptions) field.ErrorList { allErrs := ValidateObjectMeta(&pvc.ObjectMeta, true, ValidatePersistentVolumeName, field.NewPath("metadata")) @@ -2085,7 +2102,7 @@ func ValidatePersistentVolumeClaim(pvc *core.PersistentVolumeClaim, opts Persist } // validateDataSource validates a DataSource/DataSourceRef in a PersistentVolumeClaimSpec -func validateDataSource(dataSource *core.TypedLocalObjectReference, fldPath *field.Path) field.ErrorList { +func validateDataSource(dataSource *core.TypedLocalObjectReference, fldPath *field.Path, allowInvalidAPIGroupInDataSourceOrRef bool) field.ErrorList { allErrs := field.ErrorList{} if len(dataSource.Name) == 0 { @@ -2101,12 +2118,17 @@ func validateDataSource(dataSource *core.TypedLocalObjectReference, fldPath *fie if len(apiGroup) == 0 && dataSource.Kind != "PersistentVolumeClaim" { allErrs = append(allErrs, field.Invalid(fldPath, dataSource.Kind, "must be 'PersistentVolumeClaim' when referencing the default apiGroup")) } + if len(apiGroup) > 0 && !allowInvalidAPIGroupInDataSourceOrRef { + for _, errString := range validation.IsDNS1123Subdomain(apiGroup) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("apiGroup"), apiGroup, errString)) + } + } return allErrs } // validateDataSourceRef validates a DataSourceRef in a PersistentVolumeClaimSpec -func validateDataSourceRef(dataSourceRef *core.TypedObjectReference, fldPath *field.Path) field.ErrorList { +func validateDataSourceRef(dataSourceRef *core.TypedObjectReference, fldPath *field.Path, allowInvalidAPIGroupInDataSourceOrRef bool) field.ErrorList { allErrs := field.ErrorList{} if len(dataSourceRef.Name) == 0 { @@ -2122,6 +2144,11 @@ func validateDataSourceRef(dataSourceRef *core.TypedObjectReference, fldPath *fi if len(apiGroup) == 0 && dataSourceRef.Kind != "PersistentVolumeClaim" { allErrs = append(allErrs, field.Invalid(fldPath, dataSourceRef.Kind, "must be 'PersistentVolumeClaim' when referencing the default apiGroup")) } + if len(apiGroup) > 0 && !allowInvalidAPIGroupInDataSourceOrRef { + for _, errString := range validation.IsDNS1123Subdomain(apiGroup) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("apiGroup"), apiGroup, errString)) + } + } if dataSourceRef.Namespace != nil && len(*dataSourceRef.Namespace) > 0 { for _, msg := range ValidateNameFunc(ValidateNamespaceName)(*dataSourceRef.Namespace, false) { @@ -2185,10 +2212,10 @@ func ValidatePersistentVolumeClaimSpec(spec *core.PersistentVolumeClaimSpec, fld } if spec.DataSource != nil { - allErrs = append(allErrs, validateDataSource(spec.DataSource, fldPath.Child("dataSource"))...) + allErrs = append(allErrs, validateDataSource(spec.DataSource, fldPath.Child("dataSource"), opts.AllowInvalidAPIGroupInDataSourceOrRef)...) } if spec.DataSourceRef != nil { - allErrs = append(allErrs, validateDataSourceRef(spec.DataSourceRef, fldPath.Child("dataSourceRef"))...) + allErrs = append(allErrs, validateDataSourceRef(spec.DataSourceRef, fldPath.Child("dataSourceRef"), opts.AllowInvalidAPIGroupInDataSourceOrRef)...) } if spec.DataSourceRef != nil && spec.DataSourceRef.Namespace != nil && len(*spec.DataSourceRef.Namespace) > 0 { if spec.DataSource != nil { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 647cbae6a56..bb59f9cbdc9 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -996,6 +996,21 @@ func pvcTemplateWithAccessModes(accessModes []core.PersistentVolumeAccessMode) * } } +func pvcWithDataSource(dataSource *core.TypedLocalObjectReference) *core.PersistentVolumeClaim { + return &core.PersistentVolumeClaim{ + Spec: core.PersistentVolumeClaimSpec{ + DataSource: dataSource, + }, + } +} +func pvcWithDataSourceRef(ref *core.TypedObjectReference) *core.PersistentVolumeClaim { + return &core.PersistentVolumeClaim{ + Spec: core.PersistentVolumeClaimSpec{ + DataSourceRef: ref, + }, + } +} + func testLocalVolume(path string, affinity *core.VolumeNodeAffinity) core.PersistentVolumeSpec { return core.PersistentVolumeSpec{ Capacity: core.ResourceList{ @@ -1545,6 +1560,7 @@ func testVolumeClaimStorageClassInAnnotationAndNilInSpec(name, namespace, scName func testValidatePVC(t *testing.T, ephemeral bool) { invalidClassName := "-invalid-" validClassName := "valid" + invalidAPIGroup := "^invalid" invalidMode := core.PersistentVolumeMode("fakeVolumeMode") validMode := core.PersistentVolumeFilesystem goodName := "foo" @@ -1934,6 +1950,42 @@ func testValidatePVC(t *testing.T, ephemeral bool) { }, }), }, + "invaild-apigroup-in-data-source": { + isExpectedFailure: true, + claim: testVolumeClaim(goodName, goodNS, core.PersistentVolumeClaimSpec{ + AccessModes: []core.PersistentVolumeAccessMode{ + core.ReadWriteOnce, + }, + Resources: core.VolumeResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceStorage): resource.MustParse("10G"), + }, + }, + DataSource: &core.TypedLocalObjectReference{ + APIGroup: &invalidAPIGroup, + Kind: "Foo", + Name: "foo1", + }, + }), + }, + "invaild-apigroup-in-data-source-ref": { + isExpectedFailure: true, + claim: testVolumeClaim(goodName, goodNS, core.PersistentVolumeClaimSpec{ + AccessModes: []core.PersistentVolumeAccessMode{ + core.ReadWriteOnce, + }, + Resources: core.VolumeResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceStorage): resource.MustParse("10G"), + }, + }, + DataSourceRef: &core.TypedObjectReference{ + APIGroup: &invalidAPIGroup, + Kind: "Foo", + Name: "foo1", + }, + }), + }, } for name, scenario := range scenarios { @@ -2057,6 +2109,7 @@ func TestAlphaPVVolumeModeUpdate(t *testing.T) { func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { block := core.PersistentVolumeBlock file := core.PersistentVolumeFilesystem + invaildAPIGroup := "^invalid" validClaim := testVolumeClaimWithStatus("foo", "ns", core.PersistentVolumeClaimSpec{ AccessModes: []core.PersistentVolumeAccessMode{ @@ -2427,6 +2480,42 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { }, }) + invalidClaimDataSourceAPIGroup := testVolumeClaim("foo", "ns", core.PersistentVolumeClaimSpec{ + AccessModes: []core.PersistentVolumeAccessMode{ + core.ReadWriteOnce, + }, + VolumeMode: &file, + Resources: core.VolumeResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceStorage): resource.MustParse("10G"), + }, + }, + VolumeName: "volume", + DataSource: &core.TypedLocalObjectReference{ + APIGroup: &invaildAPIGroup, + Kind: "Foo", + Name: "foo", + }, + }) + + invalidClaimDataSourceRefAPIGroup := testVolumeClaim("foo", "ns", core.PersistentVolumeClaimSpec{ + AccessModes: []core.PersistentVolumeAccessMode{ + core.ReadWriteOnce, + }, + VolumeMode: &file, + Resources: core.VolumeResourceRequirements{ + Requests: core.ResourceList{ + core.ResourceName(core.ResourceStorage): resource.MustParse("10G"), + }, + }, + VolumeName: "volume", + DataSourceRef: &core.TypedObjectReference{ + APIGroup: &invaildAPIGroup, + Kind: "Foo", + Name: "foo", + }, + }) + scenarios := map[string]struct { isExpectedFailure bool oldClaim *core.PersistentVolumeClaim @@ -2631,6 +2720,16 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { enableRecoverFromExpansion: true, isExpectedFailure: true, }, + "allow-update-pvc-when-data-source-used": { + oldClaim: invalidClaimDataSourceAPIGroup, + newClaim: invalidClaimDataSourceAPIGroup, + isExpectedFailure: false, + }, + "allow-update-pvc-when-data-source-ref-used": { + oldClaim: invalidClaimDataSourceRefAPIGroup, + newClaim: invalidClaimDataSourceRefAPIGroup, + isExpectedFailure: false, + }, } for name, scenario := range scenarios { @@ -2651,6 +2750,8 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) { } func TestValidationOptionsForPersistentVolumeClaim(t *testing.T) { + invaildAPIGroup := "^invalid" + tests := map[string]struct { oldPvc *core.PersistentVolumeClaim enableReadWriteOncePod bool @@ -2696,6 +2797,18 @@ func TestValidationOptionsForPersistentVolumeClaim(t *testing.T) { EnableRecoverFromExpansionFailure: false, }, }, + "invaild apiGroup in dataSource allowed because the old pvc is used": { + oldPvc: pvcWithDataSource(&core.TypedLocalObjectReference{APIGroup: &invaildAPIGroup}), + expectValidationOpts: PersistentVolumeClaimSpecValidationOptions{ + AllowInvalidAPIGroupInDataSourceOrRef: true, + }, + }, + "invaild apiGroup in dataSourceRef allowed because the old pvc is used": { + oldPvc: pvcWithDataSourceRef(&core.TypedObjectReference{APIGroup: &invaildAPIGroup}), + expectValidationOpts: PersistentVolumeClaimSpecValidationOptions{ + AllowInvalidAPIGroupInDataSourceOrRef: true, + }, + }, } for name, tc := range tests {