Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Promote CSIPersistentVolume feature to GA #69929

Merged
merged 7 commits into from
Nov 15, 2018
Merged

Promote CSIPersistentVolume feature to GA #69929

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
534ce94
Move CSIPersistentVolume feature to GA
jsafrane Nov 14, 2018
633431c
Promote CSIPersistentVolumeSource to GA
jsafrane Nov 14, 2018
b9d9b7f
Promote VolumeAttachment to storage.k8s.io/v1 API
jsafrane Nov 14, 2018
e95b188
Generated files
jsafrane Nov 14, 2018
498cd61
Add volumeattachment status subresource
msau42 Nov 14, 2018
8cfce0a
Tighten up validation of VolumeAttachment fields
jsafrane Nov 14, 2018
eaff4fd
Tolerate existing CSI fields in PVs when the feature is disabled
liggitt Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions pkg/apis/core/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1443,25 +1443,32 @@ func validateStorageOSPersistentVolumeSource(storageos *core.StorageOSPersistent
return allErrs
}

func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
func ValidateCSIDriverName(driverName string, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate"))
if len(driverName) == 0 {
allErrs = append(allErrs, field.Required(fldPath, ""))
}

if len(csi.Driver) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("driver"), ""))
if len(driverName) > 63 {
allErrs = append(allErrs, field.TooLong(fldPath, driverName, 63))
}

if len(csi.Driver) > 63 {
allErrs = append(allErrs, field.TooLong(fldPath.Child("driver"), csi.Driver, 63))
if !csiDriverNameRexp.MatchString(driverName) {
liggitt marked this conversation as resolved.
Show resolved Hide resolved
allErrs = append(allErrs, field.Invalid(fldPath, driverName, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
}
return allErrs
}

if !csiDriverNameRexp.MatchString(csi.Driver) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("driver"), csi.Driver, validation.RegexError(csiDriverNameRexpErrMsg, csiDriverNameRexpFmt, "csi-hostpath")))
func validateCSIPersistentVolumeSource(csi *core.CSIPersistentVolumeSource, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if !utilfeature.DefaultFeatureGate.Enabled(features.CSIPersistentVolume) {
allErrs = append(allErrs, field.Forbidden(fldPath, "CSIPersistentVolume disabled by feature-gate"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this have the same issue where you can't delete the Pod object after downgrading to a version where the feature is disabled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, not the pod, since this is in PV, but the same issue, yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed a commit to https://github.com/liggitt/kubernetes/commits/csi-ga to resolve this field for this object. I think that looks like what we'll want to do more broadly (consider the existing field on update, and not block in validation based on feature enablement, since it can break update/delete scenarios)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. I will pull your commit and update this branch.

}

allErrs = append(allErrs, ValidateCSIDriverName(csi.Driver, fldPath.Child("driver"))...)

if len(csi.VolumeHandle) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("volumeHandle"), ""))
}
Expand Down
16 changes: 15 additions & 1 deletion pkg/apis/storage/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,14 +133,28 @@ func validateAllowVolumeExpansion(allowExpand *bool, fldPath *field.Path) field.
return allErrs
}

// ValidateVolumeAttachment validates a VolumeAttachment.
// ValidateVolumeAttachment validates a VolumeAttachment. This function is common for v1 and v1beta1 objects,
func ValidateVolumeAttachment(volumeAttachment *storage.VolumeAttachment) field.ErrorList {
allErrs := apivalidation.ValidateObjectMeta(&volumeAttachment.ObjectMeta, false, apivalidation.ValidateClassName, field.NewPath("metadata"))
allErrs = append(allErrs, validateVolumeAttachmentSpec(&volumeAttachment.Spec, field.NewPath("spec"))...)
allErrs = append(allErrs, validateVolumeAttachmentStatus(&volumeAttachment.Status, field.NewPath("status"))...)
return allErrs
}

// ValidateVolumeAttachmentV1 validates a v1/VolumeAttachment. It contains only extra checks missing in
// ValidateVolumeAttachment.
func ValidateVolumeAttachmentV1(volumeAttachment *storage.VolumeAttachment) field.ErrorList {
allErrs := apivalidation.ValidateCSIDriverName(volumeAttachment.Spec.Attacher, field.NewPath("spec.attacher"))

if volumeAttachment.Spec.Source.PersistentVolumeName != nil {
pvName := *volumeAttachment.Spec.Source.PersistentVolumeName
for _, msg := range apivalidation.ValidatePersistentVolumeName(pvName, false) {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec.source.persistentVolumeName"), pvName, msg))
}
}
return allErrs
}

// ValidateVolumeAttachmentSpec tests that the specified VolumeAttachmentSpec
// has valid data.
func validateVolumeAttachmentSpec(
Expand Down
72 changes: 62 additions & 10 deletions pkg/apis/storage/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,9 @@ func TestVolumeAttachmentValidation(t *testing.T) {
Spec: storage.VolumeAttachmentSpec{
Attacher: "",
NodeName: "mynode",
},
},
{
// Invalid attacher name
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
Spec: storage.VolumeAttachmentSpec{
Attacher: "invalid!@#$%^&*()",
NodeName: "mynode",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: &volumeName,
},
},
},
{
Expand All @@ -240,6 +235,9 @@ func TestVolumeAttachmentValidation(t *testing.T) {
Spec: storage.VolumeAttachmentSpec{
Attacher: "myattacher",
NodeName: "",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: &volumeName,
},
},
},
{
Expand Down Expand Up @@ -378,7 +376,7 @@ func TestVolumeAttachmentUpdateValidation(t *testing.T) {

for _, volumeAttachment := range successCases {
if errs := ValidateVolumeAttachmentUpdate(&volumeAttachment, &old); len(errs) != 0 {
t.Errorf("expected success: %v", errs)
t.Errorf("expected success: %+v", errs)
}
}

Expand Down Expand Up @@ -445,7 +443,61 @@ func TestVolumeAttachmentUpdateValidation(t *testing.T) {

for _, volumeAttachment := range errorCases {
if errs := ValidateVolumeAttachmentUpdate(&volumeAttachment, &old); len(errs) == 0 {
t.Errorf("Expected failure for test: %v", volumeAttachment)
t.Errorf("Expected failure for test: %+v", volumeAttachment)
}
}
}

func TestVolumeAttachmentValidationV1(t *testing.T) {
volumeName := "pv-name"
invalidVolumeName := "-invalid-@#$%^&*()-"
successCases := []storage.VolumeAttachment{
{
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
Spec: storage.VolumeAttachmentSpec{
Attacher: "myattacher",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: &volumeName,
},
NodeName: "mynode",
},
},
}

for _, volumeAttachment := range successCases {
if errs := ValidateVolumeAttachmentV1(&volumeAttachment); len(errs) != 0 {
t.Errorf("expected success: %+v", errs)
}
}

errorCases := []storage.VolumeAttachment{
{
// Invalid attacher name
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
Spec: storage.VolumeAttachmentSpec{
Attacher: "invalid-@#$%^&*()",
NodeName: "mynode",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: &volumeName,
},
},
},
{
// Invalid PV name
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
Spec: storage.VolumeAttachmentSpec{
Attacher: "myattacher",
NodeName: "mynode",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: &invalidVolumeName,
},
},
},
}

for _, volumeAttachment := range errorCases {
if errs := ValidateVolumeAttachmentV1(&volumeAttachment); len(errs) == 0 {
t.Errorf("Expected failure for test: %+v", volumeAttachment)
}
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/registry/storage/volumeattachment/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ go_test(
"//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
"//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
],
)
Expand Down
18 changes: 17 additions & 1 deletion pkg/registry/storage/volumeattachment/strategy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,23 @@ func (volumeAttachmentStrategy) PrepareForCreate(ctx context.Context, obj runtim

func (volumeAttachmentStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
volumeAttachment := obj.(*storage.VolumeAttachment)
return validation.ValidateVolumeAttachment(volumeAttachment)

errs := validation.ValidateVolumeAttachment(volumeAttachment)

var groupVersion schema.GroupVersion

if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
}

switch groupVersion {
case storageapiv1beta1.SchemeGroupVersion:
// no extra validation
default:
// tighten up validation of newly created v1 attachments
errs = append(errs, validation.ValidateVolumeAttachmentV1(volumeAttachment)...)
}
return errs
}

// Canonicalize normalizes the object after validation.
Expand Down
100 changes: 100 additions & 0 deletions pkg/registry/storage/volumeattachment/strategy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
apiequality "k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/diff"
"k8s.io/apimachinery/pkg/util/validation/field"
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/kubernetes/pkg/apis/storage"
)
Expand Down Expand Up @@ -195,3 +196,102 @@ func TestBetaAndV1StatusCreate(t *testing.T) {
}
}
}

func TestVolumeAttachmentValidation(t *testing.T) {
invalidPVName := "invalid-!@#$%^&*()"
validPVName := "valid-volume-name"
tests := []struct {
name string
volumeAttachment *storage.VolumeAttachment
expectBetaError bool
expectV1Error bool
}{
{
"valid attachment",
getValidVolumeAttachment("foo"),
false,
false,
},
{
"invalid PV name",
&storage.VolumeAttachment{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
},
Spec: storage.VolumeAttachmentSpec{
Attacher: "valid-attacher",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: &invalidPVName,
},
NodeName: "valid-node",
},
},
false,
true,
},
{
"invalid attacher name",
&storage.VolumeAttachment{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
},
Spec: storage.VolumeAttachmentSpec{
Attacher: "invalid!@#$%^&*()",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: &validPVName,
},
NodeName: "valid-node",
},
},
false,
true,
},
{
"invalid volume attachment",
&storage.VolumeAttachment{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
},
Spec: storage.VolumeAttachmentSpec{
Attacher: "invalid!@#$%^&*()",
Source: storage.VolumeAttachmentSource{
PersistentVolumeName: nil,
},
NodeName: "valid-node",
},
},
true,
true,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {

testValidation := func(va *storage.VolumeAttachment, apiVersion string) field.ErrorList {
ctx := genericapirequest.WithRequestInfo(genericapirequest.NewContext(), &genericapirequest.RequestInfo{
APIGroup: "storage.k8s.io",
APIVersion: apiVersion,
Resource: "volumeattachments",
})
return Strategy.Validate(ctx, va)
}

v1Err := testValidation(test.volumeAttachment, "v1")
if len(v1Err) > 0 && !test.expectV1Error {
t.Errorf("Validation of v1 object failed: %+v", v1Err)
}
if len(v1Err) == 0 && test.expectV1Error {
t.Errorf("Validation of v1 object unexpectedly succeeded")
}

betaErr := testValidation(test.volumeAttachment, "v1beta1")
if len(betaErr) > 0 && !test.expectBetaError {
t.Errorf("Validation of v1beta1 object failed: %+v", betaErr)
}
if len(betaErr) == 0 && test.expectBetaError {
t.Errorf("Validation of v1beta1 object unexpectedly succeeded")
}
})
}
}