From b52d1d474afbb6a5635281ebf3ecd333d8ded91b Mon Sep 17 00:00:00 2001 From: Shawn Hurley Date: Fri, 25 Mar 2022 13:41:55 -0400 Subject: [PATCH] add unit test coverage for v1beta1 --- pkg/validation-webhook/scheme.go | 2 +- pkg/validation-webhook/snapshot.go | 15 +- pkg/validation-webhook/snapshot_test.go | 239 ++++++++++++++++++++++++ pkg/validation-webhook/webhook.go | 6 +- pkg/validation-webhook/webhook_test.go | 17 -- 5 files changed, 251 insertions(+), 28 deletions(-) diff --git a/pkg/validation-webhook/scheme.go b/pkg/validation-webhook/scheme.go index b9819ebb..538432a0 100644 --- a/pkg/validation-webhook/scheme.go +++ b/pkg/validation-webhook/scheme.go @@ -17,7 +17,7 @@ limitations under the License. package webhook import ( - snapshot "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/scheme" + snapshot "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned/scheme" admissionv1 "k8s.io/api/admission/v1" admissionv1beta1 "k8s.io/api/admission/v1beta1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" diff --git a/pkg/validation-webhook/snapshot.go b/pkg/validation-webhook/snapshot.go index 3f46ea39..627bc3fe 100644 --- a/pkg/validation-webhook/snapshot.go +++ b/pkg/validation-webhook/snapshot.go @@ -20,10 +20,10 @@ import ( "fmt" "reflect" - volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" - volumesnapshotv1beta1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1beta1" - storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" - "github.com/kubernetes-csi/external-snapshotter/v4/pkg/utils" + volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1" + volumesnapshotv1beta1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1beta1" + storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumesnapshot/v1" + "github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils" v1 "k8s.io/api/admission/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -271,14 +271,15 @@ func decideSnapshotClassV1beta1(snapClass, oldSnapClass *volumesnapshotv1beta1.V continue } if _, ok := driverToSnapshotDefautlMap[snapshotClass.Driver]; ok { - // Log an error or something here because this means that old behavior is present. // We probably should just preserve behavior in this case. - return reviewResponse + // but not let the problem get worse. having a single one will + // suffice. + klog.V(2).Infof("snapshot driver: %v has multiple default classes", snapshotClass.Driver) } driverToSnapshotDefautlMap[snapshotClass.Driver] = snapshotClass } - if _, ok := driverToSnapshotDefautlMap[snapClass.Driver]; !ok { + if _, ok := driverToSnapshotDefautlMap[snapClass.Driver]; ok { reviewResponse.Allowed = false reviewResponse.Result.Message = fmt.Sprintf("default snapshot class already exits for driver: %v", snapClass.Driver) return reviewResponse diff --git a/pkg/validation-webhook/snapshot_test.go b/pkg/validation-webhook/snapshot_test.go index d83dc61d..69327bfd 100644 --- a/pkg/validation-webhook/snapshot_test.go +++ b/pkg/validation-webhook/snapshot_test.go @@ -23,8 +23,12 @@ import ( volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1" volumesnapshotv1beta1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1beta1" + storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumesnapshot/v1" + "github.com/kubernetes-csi/external-snapshotter/v6/pkg/utils" v1 "k8s.io/api/admission/v1" core_v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" ) @@ -702,6 +706,241 @@ func TestAdmitVolumeSnapshotContentV1(t *testing.T) { } } +type fakeSnapshotLister struct { + values []*volumesnapshotv1.VolumeSnapshotClass +} + +func (f *fakeSnapshotLister) List(selector labels.Selector) (ret []*volumesnapshotv1.VolumeSnapshotClass, err error) { + return f.values, nil +} + +func (f *fakeSnapshotLister) Get(name string) (*volumesnapshotv1.VolumeSnapshotClass, error) { + for _, v := range f.values { + if v.Name == name { + return v, nil + } + } + return nil, nil +} + func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) { + testCases := []struct { + name string + volumeSnapshotClass *volumesnapshotv1beta1.VolumeSnapshotClass + oldVolumeSnapshotClass *volumesnapshotv1beta1.VolumeSnapshotClass + shouldAdmit bool + msg string + operation v1.Operation + lister storagelisters.VolumeSnapshotClassLister + }{ + { + name: "new default for class with no existing classes", + volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{}, + shouldAdmit: true, + msg: "", + operation: v1.Create, + lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{}}, + }, + { + name: "new default for class for with existing default class different drivers", + volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{}, + shouldAdmit: true, + msg: "", + operation: v1.Create, + lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{ + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "existing.test.csi.io", + }, + }}, + }, + { + name: "new default for class with existing default class same driver", + volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{}, + shouldAdmit: false, + msg: "default snapshot class already exits for driver: test.csi.io", + operation: v1.Create, + lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{ + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + }}, + }, + { + name: "default for class with existing default class same driver update", + volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + shouldAdmit: true, + msg: "", + operation: v1.Update, + lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{ + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + }}, + }, + { + name: "new snapshot for class with existing default class same driver", + volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{}, + Driver: "test.csi.io", + }, + oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{}, + shouldAdmit: true, + msg: "", + operation: v1.Create, + lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{ + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + }}, + }, + { + name: "new snapshot for class with existing default classes", + volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{}, + shouldAdmit: false, + msg: "default snapshot class already exits for driver: test.csi.io", + operation: v1.Create, + lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{ + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + { + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + utils.IsDefaultSnapshotClassAnnotation: "true", + }, + }, + Driver: "test.csi.io", + }, + }}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + snapshotContent := tc.volumeSnapshotClass + raw, err := json.Marshal(snapshotContent) + if err != nil { + t.Fatal(err) + } + oldSnapshotClass := tc.oldVolumeSnapshotClass + oldRaw, err := json.Marshal(oldSnapshotClass) + if err != nil { + t.Fatal(err) + } + review := v1.AdmissionReview{ + Request: &v1.AdmissionRequest{ + Object: runtime.RawExtension{ + Raw: raw, + }, + OldObject: runtime.RawExtension{ + Raw: oldRaw, + }, + Resource: SnapshotClassV1Beta1GVR, + Operation: tc.operation, + }, + } + + response := admitSnapshot(review, tc.lister) + + shouldAdmit := response.Allowed + msg := response.Result.Message + + expectedResponse := tc.shouldAdmit + expectedMsg := tc.msg + + if shouldAdmit != expectedResponse { + t.Errorf("expected \"%v\" to equal \"%v\"", shouldAdmit, expectedResponse) + } + if msg != expectedMsg { + t.Errorf("expected \"%v\" to equal \"%v\"", msg, expectedMsg) + } + }) + + } } diff --git a/pkg/validation-webhook/webhook.go b/pkg/validation-webhook/webhook.go index d5d718db..df1a9afb 100644 --- a/pkg/validation-webhook/webhook.go +++ b/pkg/validation-webhook/webhook.go @@ -26,11 +26,11 @@ import ( "os" "time" - clientset "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned" - storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" + clientset "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned" + storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumesnapshot/v1" "github.com/spf13/cobra" - informers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions" + informers "github.com/kubernetes-csi/external-snapshotter/client/v6/informers/externalversions" v1 "k8s.io/api/admission/v1" "k8s.io/api/admission/v1beta1" "k8s.io/apimachinery/pkg/runtime" diff --git a/pkg/validation-webhook/webhook_test.go b/pkg/validation-webhook/webhook_test.go index 9aee1210..f1e81be5 100644 --- a/pkg/validation-webhook/webhook_test.go +++ b/pkg/validation-webhook/webhook_test.go @@ -14,22 +14,8 @@ import ( "os" "testing" "time" - - v1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" - "k8s.io/apimachinery/pkg/labels" ) -type fakeSnapshotLister struct { -} - -func (f *fakeSnapshotLister) List(selector labels.Selector) (ret []*v1.VolumeSnapshotClass, err error) { - return nil, nil -} - -func (f *fakeSnapshotLister) Get(name string) (*v1.VolumeSnapshotClass, error) { - return nil, nil -} - func TestWebhookCertReload(t *testing.T) { // Initialize test space tmpDir := os.TempDir() + "/webhook-cert-tests" @@ -46,9 +32,6 @@ func TestWebhookCertReload(t *testing.T) { t.Errorf("unexpected error occurred while deleting certs: %v", err) } }() - - //Create a fake lister - generateTestCertKeyPair(t, certFile, keyFile) // Start test server