Addressing feedback given

2022-03-28 14:15:41 -04:00
parent b52d1d474a
commit 27f8d3fe72
4 changed files with 71 additions and 169 deletions
--- a/pkg/validation-webhook/snapshot.go
+++ b/pkg/validation-webhook/snapshot.go
@@ -39,14 +39,26 @@ var (
 	SnapshotContentV1Beta1GVR = metav1.GroupVersionResource{Group: volumesnapshotv1beta1.GroupName, Version: "v1beta1", Resource: "volumesnapshotcontents"}
 	// SnapshotContentV1GVR is GroupVersionResource for v1 VolumeSnapshotContents
 	SnapshotContentV1GVR = metav1.GroupVersionResource{Group: volumesnapshotv1.GroupName, Version: "v1", Resource: "volumesnapshotcontents"}
-	// SnapshotContentV1Beta1GVR is GroupVersionResource for v1beta1 VolumeSnapshotContents
-	SnapshotClassV1Beta1GVR = metav1.GroupVersionResource{Group: volumesnapshotv1beta1.GroupName, Version: "v1beta1", Resource: "volumesnapshotclasses"}
 	// SnapshotContentV1GVR is GroupVersionResource for v1 VolumeSnapshotContents
 	SnapshotClassV1GVR = metav1.GroupVersionResource{Group: volumesnapshotv1.GroupName, Version: "v1", Resource: "volumesnapshotclasses"}
 )

+type SnapshotAdmitter interface {
+	Admit(v1.AdmissionReview) *v1.AdmissionResponse
+}
+
+type admitter struct {
+	lister storagelisters.VolumeSnapshotClassLister
+}
+
+func NewSnapshotAdmitter(lister storagelisters.VolumeSnapshotClassLister) SnapshotAdmitter {
+	return &admitter{
+		lister: lister,
+	}
+}
+
 // Add a label {"added-label": "yes"} to the object
-func admitSnapshot(ar v1.AdmissionReview, lister storagelisters.VolumeSnapshotClassLister) *v1.AdmissionResponse {
+func (a admitter) Admit(ar v1.AdmissionReview) *v1.AdmissionResponse {
 	klog.V(2).Info("admitting volumesnapshots or volumesnapshotcontents")

 	reviewResponse := &v1.AdmissionResponse{
@@ -113,18 +125,18 @@ func admitSnapshot(ar v1.AdmissionReview, lister storagelisters.VolumeSnapshotCl
 			return toV1AdmissionResponse(err)
 		}
 		return decideSnapshotContentV1(snapcontent, oldSnapcontent, isUpdate)
-	case SnapshotClassV1Beta1GVR:
-		snapClass := &volumesnapshotv1beta1.VolumeSnapshotClass{}
+	case SnapshotClassV1GVR:
+		snapClass := &volumesnapshotv1.VolumeSnapshotClass{}
 		if _, _, err := deserializer.Decode(raw, nil, snapClass); err != nil {
 			klog.Error(err)
 			return toV1AdmissionResponse(err)
 		}
-		oldSnapClass := &volumesnapshotv1beta1.VolumeSnapshotClass{}
+		oldSnapClass := &volumesnapshotv1.VolumeSnapshotClass{}
 		if _, _, err := deserializer.Decode(oldRaw, nil, oldSnapClass); err != nil {
 			klog.Error(err)
 			return toV1AdmissionResponse(err)
 		}
-		return decideSnapshotClassV1beta1(snapClass, oldSnapClass, lister)
+		return decideSnapshotClassV1(snapClass, oldSnapClass, a.lister)
 	default:
 		err := fmt.Errorf("expect resource to be %s or %s", SnapshotV1Beta1GVR, SnapshotContentV1Beta1GVR)
 		klog.Error(err)
@@ -242,7 +254,7 @@ func decideSnapshotContentV1(snapcontent, oldSnapcontent *volumesnapshotv1.Volum
 	return reviewResponse
 }

-func decideSnapshotClassV1beta1(snapClass, oldSnapClass *volumesnapshotv1beta1.VolumeSnapshotClass, lister storagelisters.VolumeSnapshotClassLister) *v1.AdmissionResponse {
+func decideSnapshotClassV1(snapClass, oldSnapClass *volumesnapshotv1.VolumeSnapshotClass, lister storagelisters.VolumeSnapshotClassLister) *v1.AdmissionResponse {
 	reviewResponse := &v1.AdmissionResponse{
 		Allowed: true,
 		Result:  &metav1.Status{},
@@ -265,25 +277,17 @@ func decideSnapshotClassV1beta1(snapClass, oldSnapClass *volumesnapshotv1beta1.V
 		return reviewResponse
 	}

-	driverToSnapshotDefautlMap := map[string]*volumesnapshotv1.VolumeSnapshotClass{}
 	for _, snapshotClass := range ret {
 		if snapshotClass.Annotations[utils.IsDefaultSnapshotClassAnnotation] != "true" {
 			continue
 		}
-		if _, ok := driverToSnapshotDefautlMap[snapshotClass.Driver]; ok {
-			// We probably should just preserve behavior in this case.
-			// 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)
+		if snapshotClass.Driver == snapClass.Driver {
+			reviewResponse.Allowed = false
+			reviewResponse.Result.Message = fmt.Sprintf("default snapshot class: %v already exits for driver: %v", snapshotClass.Name, snapClass.Driver)
+			return reviewResponse
 		}
-		driverToSnapshotDefautlMap[snapshotClass.Driver] = snapshotClass
 	}

-	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
-	}
 	return reviewResponse
 }

--- a/pkg/validation-webhook/snapshot_test.go
+++ b/pkg/validation-webhook/snapshot_test.go
@@ -212,7 +212,8 @@ func TestAdmitVolumeSnapshotV1beta1(t *testing.T) {
 					Operation: tc.operation,
 				},
 			}
-			response := admitSnapshot(review, nil)
+			sa := NewSnapshotAdmitter(nil)
+			response := sa.Admit(review)
 			shouldAdmit := response.Allowed
 			msg := response.Result.Message

@@ -395,7 +396,8 @@ func TestAdmitVolumeSnapshotV1(t *testing.T) {
 					Operation: tc.operation,
 				},
 			}
-			response := admitSnapshot(review, nil)
+			sa := NewSnapshotAdmitter(nil)
+			response := sa.Admit(review)
 			shouldAdmit := response.Allowed
 			msg := response.Result.Message

@@ -545,7 +547,8 @@ func TestAdmitVolumeSnapshotContentV1beta1(t *testing.T) {
 					Operation: tc.operation,
 				},
 			}
-			response := admitSnapshot(review, nil)
+			sa := NewSnapshotAdmitter(nil)
+			response := sa.Admit(review)
 			shouldAdmit := response.Allowed
 			msg := response.Result.Message

@@ -689,7 +692,8 @@ func TestAdmitVolumeSnapshotContentV1(t *testing.T) {
 					Operation: tc.operation,
 				},
 			}
-			response := admitSnapshot(review, nil)
+			sa := NewSnapshotAdmitter(nil)
+			response := sa.Admit(review)
 			shouldAdmit := response.Allowed
 			msg := response.Result.Message

@@ -723,11 +727,11 @@ func (f *fakeSnapshotLister) Get(name string) (*volumesnapshotv1.VolumeSnapshotC
 	return nil, nil
 }

-func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
+func TestAdmitVolumeSnapshotClassV1(t *testing.T) {
 	testCases := []struct {
 		name                   string
-		volumeSnapshotClass    *volumesnapshotv1beta1.VolumeSnapshotClass
-		oldVolumeSnapshotClass *volumesnapshotv1beta1.VolumeSnapshotClass
+		volumeSnapshotClass    *volumesnapshotv1.VolumeSnapshotClass
+		oldVolumeSnapshotClass *volumesnapshotv1.VolumeSnapshotClass
 		shouldAdmit            bool
 		msg                    string
 		operation              v1.Operation
@@ -735,7 +739,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 	}{
 		{
 			name: "new default for class with no existing classes",
-			volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{
+			volumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{
 				TypeMeta: metav1.TypeMeta{},
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
@@ -744,7 +748,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 				},
 				Driver: "test.csi.io",
 			},
-			oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{},
+			oldVolumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{},
 			shouldAdmit:            true,
 			msg:                    "",
 			operation:              v1.Create,
@@ -752,7 +756,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 		},
 		{
 			name: "new default for class for  with existing default class different drivers",
-			volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{
+			volumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{
 				TypeMeta: metav1.TypeMeta{},
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
@@ -761,7 +765,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 				},
 				Driver: "test.csi.io",
 			},
-			oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{},
+			oldVolumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{},
 			shouldAdmit:            true,
 			msg:                    "",
 			operation:              v1.Create,
@@ -779,7 +783,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 		},
 		{
 			name: "new default for class with existing default class same driver",
-			volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{
+			volumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{
 				TypeMeta: metav1.TypeMeta{},
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
@@ -788,14 +792,15 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 				},
 				Driver: "test.csi.io",
 			},
-			oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{},
+			oldVolumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{},
 			shouldAdmit:            false,
-			msg:                    "default snapshot class already exits for driver: test.csi.io",
+			msg:                    "default snapshot class: driver-a already exits for driver: test.csi.io",
 			operation:              v1.Create,
 			lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{
 				{
 					TypeMeta: metav1.TypeMeta{},
 					ObjectMeta: metav1.ObjectMeta{
+						Name: "driver-a",
 						Annotations: map[string]string{
 							utils.IsDefaultSnapshotClassAnnotation: "true",
 						},
@@ -806,7 +811,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 		},
 		{
 			name: "default for class with existing default class same driver update",
-			volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{
+			volumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{
 				TypeMeta: metav1.TypeMeta{},
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
@@ -815,7 +820,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 				},
 				Driver: "test.csi.io",
 			},
-			oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{
+			oldVolumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{
 				TypeMeta: metav1.TypeMeta{},
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
@@ -841,12 +846,12 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 		},
 		{
 			name: "new snapshot for class with existing default class same driver",
-			volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{
+			volumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{
 				TypeMeta:   metav1.TypeMeta{},
 				ObjectMeta: metav1.ObjectMeta{},
 				Driver:     "test.csi.io",
 			},
-			oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{},
+			oldVolumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{},
 			shouldAdmit:            true,
 			msg:                    "",
 			operation:              v1.Create,
@@ -864,7 +869,7 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 		},
 		{
 			name: "new snapshot for class with existing default classes",
-			volumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{
+			volumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{
 				TypeMeta: metav1.TypeMeta{},
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{
@@ -873,14 +878,15 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 				},
 				Driver: "test.csi.io",
 			},
-			oldVolumeSnapshotClass: &volumesnapshotv1beta1.VolumeSnapshotClass{},
+			oldVolumeSnapshotClass: &volumesnapshotv1.VolumeSnapshotClass{},
 			shouldAdmit:            false,
-			msg:                    "default snapshot class already exits for driver: test.csi.io",
+			msg:                    "default snapshot class: driver-is-default already exits for driver: test.csi.io",
 			operation:              v1.Create,
 			lister: &fakeSnapshotLister{values: []*volumesnapshotv1.VolumeSnapshotClass{
 				{
 					TypeMeta: metav1.TypeMeta{},
 					ObjectMeta: metav1.ObjectMeta{
+						Name: "driver-is-default",
 						Annotations: map[string]string{
 							utils.IsDefaultSnapshotClassAnnotation: "true",
 						},
@@ -920,12 +926,12 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 					OldObject: runtime.RawExtension{
 						Raw: oldRaw,
 					},
-					Resource:  SnapshotClassV1Beta1GVR,
+					Resource:  SnapshotClassV1GVR,
 					Operation: tc.operation,
 				},
 			}
-
-			response := admitSnapshot(review, tc.lister)
+			sa := NewSnapshotAdmitter(tc.lister)
+			response := sa.Admit(review)

 			shouldAdmit := response.Allowed
 			msg := response.Result.Message
@@ -940,7 +946,5 @@ func TestAdmitVolumeSnapshotClassV1beta1(t *testing.T) {
 				t.Errorf("expected \"%v\" to equal \"%v\"", msg, expectedMsg)
 			}
 		})
-
 	}
-
 }
--- a/pkg/validation-webhook/webhook.go
+++ b/pkg/validation-webhook/webhook.go
@@ -24,7 +24,6 @@ import (
 	"io/ioutil"
 	"net/http"
 	"os"
-	"time"

 	clientset "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned"
 	storagelisters "github.com/kubernetes-csi/external-snapshotter/client/v6/listers/volumesnapshot/v1"
@@ -71,35 +70,33 @@ func init() {
 }

 // admitv1beta1Func handles a v1beta1 admission
-type admitv1beta1Func func(v1beta1.AdmissionReview, storagelisters.VolumeSnapshotClassLister) *v1beta1.AdmissionResponse
+type admitv1beta1Func func(v1beta1.AdmissionReview) *v1beta1.AdmissionResponse

 // admitv1beta1Func handles a v1 admission
-type admitv1Func func(v1.AdmissionReview, storagelisters.VolumeSnapshotClassLister) *v1.AdmissionResponse
+type admitv1Func func(v1.AdmissionReview) *v1.AdmissionResponse

 // admitHandler is a handler, for both validators and mutators, that supports multiple admission review versions
 type admitHandler struct {
-	v1beta1 admitv1beta1Func
-	v1      admitv1Func
+	SnapshotAdmitter
 }

-func newDelegateToV1AdmitHandler(f admitv1Func) admitHandler {
+func newDelegateToV1AdmitHandler(sa SnapshotAdmitter) admitHandler {
 	return admitHandler{
-		v1beta1: delegateV1beta1AdmitToV1(f),
-		v1:      f,
+		SnapshotAdmitter: sa,
 	}
 }

 func delegateV1beta1AdmitToV1(f admitv1Func) admitv1beta1Func {
-	return func(review v1beta1.AdmissionReview, lister storagelisters.VolumeSnapshotClassLister) *v1beta1.AdmissionResponse {
+	return func(review v1beta1.AdmissionReview) *v1beta1.AdmissionResponse {
 		in := v1.AdmissionReview{Request: convertAdmissionRequestToV1(review.Request)}
-		out := f(in, lister)
+		out := f(in)
 		return convertAdmissionResponseToV1beta1(out)
 	}
 }

 // serve handles the http portion of a request prior to handing to an admit
 // function
-func serve(w http.ResponseWriter, r *http.Request, admit admitHandler, lister storagelisters.VolumeSnapshotClassLister) {
+func serve(w http.ResponseWriter, r *http.Request, admit admitHandler) {
 	var body []byte
 	if r.Body == nil {
 		msg := "Expected request body to be non-empty"
@@ -147,7 +144,7 @@ func serve(w http.ResponseWriter, r *http.Request, admit admitHandler, lister st
 		}
 		responseAdmissionReview := &v1beta1.AdmissionReview{}
 		responseAdmissionReview.SetGroupVersionKind(*gvk)
-		responseAdmissionReview.Response = admit.v1beta1(*requestedAdmissionReview, lister)
+		responseAdmissionReview.Response = delegateV1beta1AdmitToV1(admit.Admit)(*requestedAdmissionReview)
 		responseAdmissionReview.Response.UID = requestedAdmissionReview.Request.UID
 		responseObj = responseAdmissionReview
 	case v1.SchemeGroupVersion.WithKind("AdmissionReview"):
@@ -160,7 +157,7 @@ func serve(w http.ResponseWriter, r *http.Request, admit admitHandler, lister st
 		}
 		responseAdmissionReview := &v1.AdmissionReview{}
 		responseAdmissionReview.SetGroupVersionKind(*gvk)
-		responseAdmissionReview.Response = admit.v1(*requestedAdmissionReview, lister)
+		responseAdmissionReview.Response = admit.Admit(*requestedAdmissionReview)
 		responseAdmissionReview.Response.UID = requestedAdmissionReview.Request.UID
 		responseObj = responseAdmissionReview
 	default:
@@ -188,7 +185,7 @@ type serveWebhook struct {
 }

 func (s serveWebhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	serve(w, r, newDelegateToV1AdmitHandler(admitSnapshot), s.lister)
+	serve(w, r, newDelegateToV1AdmitHandler(NewSnapshotAdmitter(s.lister)))
 }

 func startServer(ctx context.Context, tlsConfig *tls.Config, cw *CertWatcher, lister storagelisters.VolumeSnapshotClassLister) error {
@@ -246,12 +243,14 @@ func main(cmd *cobra.Command, args []string) {
 		os.Exit(1)
 	}

-	r := 0 * time.Second
-	resyncPeriod := &r
-
-	factory := informers.NewSharedInformerFactory(snapClient, *resyncPeriod)
+	factory := informers.NewSharedInformerFactory(snapClient, 0)
 	lister := factory.Snapshot().V1().VolumeSnapshotClasses().Lister()

+	//Start the informers
+	factory.Start(ctx.Done())
+	//wait for the caches to sync
+	factory.WaitForCacheSync(ctx.Done())
+
 	if err := startServer(ctx, tlsConfig, cw, lister); err != nil {
 		klog.Fatalf("server stopped: %v", err)
 	}