diff --git a/pkg/registry/storage/volumeattachment/strategy.go b/pkg/registry/storage/volumeattachment/strategy.go index 2e4182f2294..26bd749286a 100644 --- a/pkg/registry/storage/volumeattachment/strategy.go +++ b/pkg/registry/storage/volumeattachment/strategy.go @@ -19,12 +19,9 @@ package volumeattachment import ( "context" - storageapiv1beta1 "k8s.io/api/storage/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" - genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" @@ -62,20 +59,8 @@ func (volumeAttachmentStrategy) GetResetFields() map[fieldpath.APIVersion]*field // ResetBeforeCreate clears the Status field which is not allowed to be set by end users on creation. func (volumeAttachmentStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { - var groupVersion schema.GroupVersion - - if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { - groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} - } - volumeAttachment := obj.(*storage.VolumeAttachment) - - switch groupVersion { - case storageapiv1beta1.SchemeGroupVersion: - // allow modification of status for v1beta1 - default: - volumeAttachment.Status = storage.VolumeAttachmentStatus{} - } + volumeAttachment.Status = storage.VolumeAttachmentStatus{} if !utilfeature.DefaultFeatureGate.Enabled(features.CSIMigration) { volumeAttachment.Spec.Source.InlineVolumeSpec = nil @@ -88,19 +73,8 @@ func (volumeAttachmentStrategy) Validate(ctx context.Context, obj runtime.Object 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)...) - } + // tighten up validation of newly created v1 attachments + errs = append(errs, validation.ValidateVolumeAttachmentV1(volumeAttachment)...) return errs } @@ -119,22 +93,11 @@ func (volumeAttachmentStrategy) AllowCreateOnUpdate() bool { // PrepareForUpdate sets the Status fields which is not allowed to be set by an end user updating a VolumeAttachment func (volumeAttachmentStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { - var groupVersion schema.GroupVersion - - if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { - groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} - } - newVolumeAttachment := obj.(*storage.VolumeAttachment) oldVolumeAttachment := old.(*storage.VolumeAttachment) - switch groupVersion { - case storageapiv1beta1.SchemeGroupVersion: - // allow modification of Status via main resource for v1beta1 - default: - newVolumeAttachment.Status = oldVolumeAttachment.Status - // No need to increment Generation because we don't allow updates to spec - } + newVolumeAttachment.Status = oldVolumeAttachment.Status + // No need to increment Generation because we don't allow updates to spec if !utilfeature.DefaultFeatureGate.Enabled(features.CSIMigration) && oldVolumeAttachment.Spec.Source.InlineVolumeSpec == nil { newVolumeAttachment.Spec.Source.InlineVolumeSpec = nil diff --git a/pkg/registry/storage/volumeattachment/strategy_test.go b/pkg/registry/storage/volumeattachment/strategy_test.go index 9ee8e73fb9e..a641761a764 100644 --- a/pkg/registry/storage/volumeattachment/strategy_test.go +++ b/pkg/registry/storage/volumeattachment/strategy_test.go @@ -17,13 +17,13 @@ limitations under the License. package volumeattachment import ( + "context" "testing" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/resource" 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" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" @@ -196,77 +196,22 @@ func TestVolumeAttachmentStatusStrategy(t *testing.T) { } } -func TestBetaAndV1StatusUpdate(t *testing.T) { - tests := []struct { - requestInfo genericapirequest.RequestInfo - newStatus bool - expectedStatus bool - }{ - { - genericapirequest.RequestInfo{ - APIGroup: "storage.k8s.io", - APIVersion: "v1", - Resource: "volumeattachments", - }, - true, - false, - }, - { - genericapirequest.RequestInfo{ - APIGroup: "storage.k8s.io", - APIVersion: "v1beta1", - Resource: "volumeattachments", - }, - true, - true, - }, +func TestUpdatePreventsStatusWrite(t *testing.T) { + va := getValidVolumeAttachment("valid-attachment") + newAttachment := va.DeepCopy() + newAttachment.Status.Attached = true + Strategy.PrepareForUpdate(context.TODO(), newAttachment, va) + if newAttachment.Status.Attached { + t.Errorf("expected status to be %v got %v", false, newAttachment.Status.Attached) } - for _, test := range tests { - va := getValidVolumeAttachment("valid-attachment") - newAttachment := va.DeepCopy() - newAttachment.Status.Attached = test.newStatus - context := genericapirequest.WithRequestInfo(genericapirequest.NewContext(), &test.requestInfo) - Strategy.PrepareForUpdate(context, newAttachment, va) - if newAttachment.Status.Attached != test.expectedStatus { - t.Errorf("expected status to be %v got %v", test.expectedStatus, newAttachment.Status.Attached) - } - } - } -func TestBetaAndV1StatusCreate(t *testing.T) { - tests := []struct { - requestInfo genericapirequest.RequestInfo - newStatus bool - expectedStatus bool - }{ - { - genericapirequest.RequestInfo{ - APIGroup: "storage.k8s.io", - APIVersion: "v1", - Resource: "volumeattachments", - }, - true, - false, - }, - { - genericapirequest.RequestInfo{ - APIGroup: "storage.k8s.io", - APIVersion: "v1beta1", - Resource: "volumeattachments", - }, - true, - true, - }, - } - for _, test := range tests { - va := getValidVolumeAttachment("valid-attachment") - va.Status.Attached = test.newStatus - context := genericapirequest.WithRequestInfo(genericapirequest.NewContext(), &test.requestInfo) - Strategy.PrepareForCreate(context, va) - if va.Status.Attached != test.expectedStatus { - t.Errorf("expected status to be %v got %v", test.expectedStatus, va.Status.Attached) - } +func TestCreatePreventsStatusWrite(t *testing.T) { + va := getValidVolumeAttachment("valid-attachment") + va.Status.Attached = true + Strategy.PrepareForCreate(context.TODO(), va) + if va.Status.Attached { + t.Errorf("expected status to be false got %v", va.Status.Attached) } } @@ -276,14 +221,12 @@ func TestVolumeAttachmentValidation(t *testing.T) { tests := []struct { name string volumeAttachment *storage.VolumeAttachment - expectBetaError bool - expectV1Error bool + expectError bool }{ { "valid attachment", getValidVolumeAttachment("foo"), false, - false, }, { "invalid PV name", @@ -299,7 +242,6 @@ func TestVolumeAttachmentValidation(t *testing.T) { NodeName: "valid-node", }, }, - false, true, }, { @@ -316,7 +258,6 @@ func TestVolumeAttachmentValidation(t *testing.T) { NodeName: "valid-node", }, }, - false, true, }, { @@ -334,36 +275,17 @@ func TestVolumeAttachmentValidation(t *testing.T) { }, }, 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) + err := Strategy.Validate(context.TODO(), test.volumeAttachment) + if len(err) > 0 && !test.expectError { + t.Errorf("Validation of object failed: %+v", err) } - - 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") + if len(err) == 0 && test.expectError { + t.Errorf("Validation of object unexpectedly succeeded") } }) }