From 5c92e4b81617130df8549ff3cf5522a617e8a1f1 Mon Sep 17 00:00:00 2001 From: Humble Chirammal Date: Tue, 19 Jul 2022 09:51:14 +0530 Subject: [PATCH 1/2] csi: validate the secretnames in the CSI spec against NameIsDNSSubdomain At present the CSI spec secret name validation for ControllerPublish, ControllerExpand, NodePublish secrets are performed against ValidateDNS1123Label() and it causes the secret name validation inside the CSI spec to go wrong if the secret name is more than 63 chars. Kubernetes allow the secret object name to be on `DNS SubDomainName` and having a secret name length between 0-253 is correct/valid. So the CSI spec validation also has to be performed accordingly. This commit address this issue in validation for above mentioned funcs. Signed-off-by: Humble Chirammal --- pkg/apis/core/validation/validation.go | 34 ++++++++++++++++++++------ 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 0e07a8e2a95..e8541e63bbf 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -1532,13 +1532,16 @@ func validateStorageOSPersistentVolumeSource(storageos *core.StorageOSPersistent // validatePVSecretReference check whether provided SecretReference object is valid in terms of secret name and namespace. -func validatePVSecretReference(secretRef *core.SecretReference, fldPath *field.Path) field.ErrorList { +func validatePVSecretReference(secretRef *core.SecretReference, allowDNSSubDomainSecretName bool, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList if len(secretRef.Name) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) + } else if allowDNSSubDomainSecretName { + allErrs = append(allErrs, ValidateDNS1123Subdomain(secretRef.Name, fldPath.Child("name"))...) } else { allErrs = append(allErrs, ValidateDNS1123Label(secretRef.Name, fldPath.Child("name"))...) } + if len(secretRef.Namespace) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("namespace"), "")) } else { @@ -1565,7 +1568,7 @@ func ValidateCSIDriverName(driverName string, fldPath *field.Path) field.ErrorLi return allErrs } -func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList { +func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, allowDNSSubDomainSecretName bool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, ValidateCSIDriverName(csi.Driver, fldPath.Child("driver"))...) @@ -1574,16 +1577,16 @@ func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldP allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), "")) } if csi.ControllerPublishSecretRef != nil { - allErrs = append(allErrs, validatePVSecretReference(csi.ControllerPublishSecretRef, fldPath.Child("controllerPublishSecretRef"))...) + allErrs = append(allErrs, validatePVSecretReference(csi.ControllerPublishSecretRef, allowDNSSubDomainSecretName, fldPath.Child("controllerPublishSecretRef"))...) } if csi.ControllerExpandSecretRef != nil { - allErrs = append(allErrs, validatePVSecretReference(csi.ControllerExpandSecretRef, fldPath.Child("controllerExpandSecretRef"))...) + allErrs = append(allErrs, validatePVSecretReference(csi.ControllerExpandSecretRef, allowDNSSubDomainSecretName, fldPath.Child("controllerExpandSecretRef"))...) } if csi.NodePublishSecretRef != nil { - allErrs = append(allErrs, validatePVSecretReference(csi.NodePublishSecretRef, fldPath.Child("nodePublishSecretRef"))...) + allErrs = append(allErrs, validatePVSecretReference(csi.NodePublishSecretRef, allowDNSSubDomainSecretName, fldPath.Child("nodePublishSecretRef"))...) } if csi.NodeExpandSecretRef != nil { - allErrs = append(allErrs, validatePVSecretReference(csi.NodeExpandSecretRef, fldPath.Child("nodeExpandSecretRef"))...) + allErrs = append(allErrs, validatePVSecretReference(csi.NodeExpandSecretRef, allowDNSSubDomainSecretName, fldPath.Child("nodeExpandSecretRef"))...) } return allErrs } @@ -1645,6 +1648,8 @@ var allowedPVCTemplateObjectMetaFields = map[string]bool{ type PersistentVolumeSpecValidationOptions struct { // Allow spec to contain the "ReadWiteOncePod" access mode AllowReadWriteOncePod bool + // Allow the secretRef Name field to be of DNSSubDomain Format + AllowDNSSubDomainSecretName bool } // ValidatePersistentVolumeName checks that a name is appropriate for a @@ -1659,7 +1664,8 @@ var supportedVolumeModes = sets.NewString(string(core.PersistentVolumeBlock), st func ValidationOptionsForPersistentVolume(pv, oldPv *core.PersistentVolume) PersistentVolumeSpecValidationOptions { opts := PersistentVolumeSpecValidationOptions{ - AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), + AllowReadWriteOncePod: utilfeature.DefaultFeatureGate.Enabled(features.ReadWriteOncePod), + AllowDNSSubDomainSecretName: false, } if oldPv == nil { // If there's no old PV, use the options based solely on feature enablement @@ -1669,9 +1675,21 @@ func ValidationOptionsForPersistentVolume(pv, oldPv *core.PersistentVolume) Pers // If the old object allowed "ReadWriteOncePod", continue to allow it in the new object opts.AllowReadWriteOncePod = true } + if oldCSI := oldPv.Spec.CSI; oldCSI != nil { + opts.AllowDNSSubDomainSecretName = + secretRefRequiresSubdomainSecretName(oldCSI.ControllerExpandSecretRef) || + secretRefRequiresSubdomainSecretName(oldCSI.ControllerPublishSecretRef) || + secretRefRequiresSubdomainSecretName(oldCSI.NodeStageSecretRef) || + secretRefRequiresSubdomainSecretName(oldCSI.NodePublishSecretRef) + } return opts } +func secretRefRequiresSubdomainSecretName(secretRef *core.SecretReference) bool { + // ref and name were specified and name didn't fit within label validation + return secretRef != nil && len(secretRef.Name) > 0 && len(validation.IsDNS1123Label(secretRef.Name)) > 0 +} + func ValidatePersistentVolumeSpec(pvSpec *core.PersistentVolumeSpec, pvName string, validateInlinePersistentVolumeSpec bool, fldPath *field.Path, opts PersistentVolumeSpecValidationOptions) field.ErrorList { allErrs := field.ErrorList{} @@ -1926,7 +1944,7 @@ func ValidatePersistentVolumeSpec(pvSpec *core.PersistentVolumeSpec, pvName stri allErrs = append(allErrs, field.Forbidden(fldPath.Child("csi"), "may not specify more than 1 volume type")) } else { numVolumes++ - allErrs = append(allErrs, validateCSIPersistentVolumeSource(pvSpec.CSI, fldPath.Child("csi"))...) + allErrs = append(allErrs, validateCSIPersistentVolumeSource(pvSpec.CSI, opts.AllowDNSSubDomainSecretName, fldPath.Child("csi"))...) } } From e2ab0f93e6a47c2a492a8d0e49ba632a1ae28987 Mon Sep 17 00:00:00 2001 From: Humble Chirammal Date: Tue, 19 Jul 2022 09:51:53 +0530 Subject: [PATCH 2/2] Add unit tests for allowSubDomainSecret format validation Signed-off-by: Humble Chirammal --- pkg/apis/core/validation/validation_test.go | 97 +++++++++++++++++++-- 1 file changed, 88 insertions(+), 9 deletions(-) diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 75e4ef413ff..d0b61fe5e4a 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -759,7 +759,7 @@ func TestValidatePersistentVolumeSourceUpdate(t *testing.T) { newVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "controllerExpand"), }, "csi-expansion-enabled-from-longSecretRef-to-longSecretRef": { - isExpectedFailure: true, + isExpectedFailure: false, oldVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "controllerExpand"), newVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "controllerExpand"), }, @@ -784,7 +784,7 @@ func TestValidatePersistentVolumeSourceUpdate(t *testing.T) { newVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "controllerPublish"), }, "csi-cntrlpublish-enabled-from-longSecretRef-to-longSecretRef": { - isExpectedFailure: true, + isExpectedFailure: false, oldVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "controllerPublish"), newVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "controllerPublish"), }, @@ -809,7 +809,7 @@ func TestValidatePersistentVolumeSourceUpdate(t *testing.T) { newVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "nodePublish"), }, "csi-nodepublish-enabled-from-longSecretRef-to-longSecretRef": { - isExpectedFailure: true, + isExpectedFailure: false, oldVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "nodePublish"), newVolume: getCSIVolumeWithSecret(validCSIVolume, longSecretRef, "nodePublish"), }, @@ -2745,10 +2745,11 @@ func TestValidateCSIVolumeSource(t *testing.T) { func TestValidateCSIPersistentVolumeSource(t *testing.T) { testCases := []struct { - name string - csi *core.CSIPersistentVolumeSource - errtype field.ErrorType - errfield string + name string + csi *core.CSIPersistentVolumeSource + errtype field.ErrorType + errfield string + allowDNSSubDomainSecretName bool }{ { name: "all required fields ok", @@ -2906,10 +2907,88 @@ func TestValidateCSIPersistentVolumeSource(t *testing.T) { name: "valid nodeExpandSecretRef", csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodeExpandSecretRef: &core.SecretReference{Name: "foobar", Namespace: "default"}}, }, + { + name: "Invalid nodePublishSecretRef", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodePublishSecretRef: &core.SecretReference{Name: "foobar", Namespace: "default"}}, + }, + + // tests with allowDNSSubDomainSecretName flag on/off + { + name: "valid nodeExpandSecretRef with allow flag off", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodeExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 63), Namespace: "default"}}, + allowDNSSubDomainSecretName: false, + }, + { + name: "Invalid nodeExpandSecretRef with allow flag off", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodeExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 65), Namespace: "default"}}, + allowDNSSubDomainSecretName: false, + errtype: field.ErrorTypeInvalid, + errfield: "nodeExpandSecretRef.name", + }, + { + name: "valid nodeExpandSecretRef with allow flag on", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodeExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 65), Namespace: "default"}}, + allowDNSSubDomainSecretName: true, + }, + { + name: "Invalid nodeExpandSecretRef with allow flag on", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodeExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 255), Namespace: "default"}}, + allowDNSSubDomainSecretName: true, + errtype: field.ErrorTypeInvalid, + errfield: "nodeExpandSecretRef.name", + }, + { + name: "valid nodePublishSecretRef with allow flag off", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodePublishSecretRef: &core.SecretReference{Name: strings.Repeat("g", 63), Namespace: "default"}}, + allowDNSSubDomainSecretName: false, + }, + { + name: "Invalid nodePublishSecretRef with allow flag off", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodePublishSecretRef: &core.SecretReference{Name: strings.Repeat("g", 65), Namespace: "default"}}, + allowDNSSubDomainSecretName: false, + errtype: field.ErrorTypeInvalid, + errfield: "nodePublishSecretRef.name", + }, + { + name: "valid nodePublishSecretRef with allow flag on", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodePublishSecretRef: &core.SecretReference{Name: strings.Repeat("g", 65), Namespace: "default"}}, + allowDNSSubDomainSecretName: true, + }, + { + name: "Invalid nodePublishSecretRef with allow flag on", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", NodePublishSecretRef: &core.SecretReference{Name: strings.Repeat("g", 255), Namespace: "default"}}, + allowDNSSubDomainSecretName: true, + errtype: field.ErrorTypeInvalid, + errfield: "nodePublishSecretRef.name", + }, + { + name: "valid ControllerExpandSecretRef with allow flag off", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", ControllerExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 63), Namespace: "default"}}, + allowDNSSubDomainSecretName: false, + }, + { + name: "Invalid ControllerExpandSecretRef with allow flag off", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", ControllerExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 65), Namespace: "default"}}, + allowDNSSubDomainSecretName: false, + errtype: field.ErrorTypeInvalid, + errfield: "controllerExpandSecretRef.name", + }, + { + name: "valid ControllerExpandSecretRef with allow flag on", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", ControllerExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 65), Namespace: "default"}}, + allowDNSSubDomainSecretName: true, + }, + { + name: "Invalid ControllerExpandSecretRef with allow flag on", + csi: &core.CSIPersistentVolumeSource{Driver: "com.google.gcepd", VolumeHandle: "foobar", ControllerExpandSecretRef: &core.SecretReference{Name: strings.Repeat("g", 255), Namespace: "default"}}, + allowDNSSubDomainSecretName: true, + errtype: field.ErrorTypeInvalid, + errfield: "controllerExpandSecretRef.name", + }, } for i, tc := range testCases { - errs := validateCSIPersistentVolumeSource(tc.csi, field.NewPath("field")) + errs := validateCSIPersistentVolumeSource(tc.csi, tc.allowDNSSubDomainSecretName, field.NewPath("field")) if len(errs) > 0 && tc.errtype == "" { t.Errorf("[%d: %q] unexpected error(s): %v", i, tc.name, errs) @@ -20559,7 +20638,7 @@ func TestValidatePVSecretReference(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - errs := validatePVSecretReference(tt.args.secretRef, tt.args.fldPath) + errs := validatePVSecretReference(tt.args.secretRef, false, tt.args.fldPath) if tt.expectError && len(errs) == 0 { t.Errorf("Unexpected success") }