Conversion webhook review

Signed-off-by: Leonardo Cecchi <[email protected]>

Author: leonardoce

cmd/snapshot-conversion-webhook/main.go

@@ -21,11 +21,12 @@ import (
 	"crypto/tls"
 	"flag"
 
-	"github.com/kubernetes-csi/csi-lib-utils/standardflags"
-	"github.com/kubernetes-csi/external-snapshotter/v8/pkg/webhook"
 	"k8s.io/component-base/logs"
 	logsapi "k8s.io/component-base/logs/api/v1"
 	"k8s.io/klog/v2"
+
+	"github.com/kubernetes-csi/csi-lib-utils/standardflags"
+	"github.com/kubernetes-csi/external-snapshotter/v8/pkg/webhook"
 )
 
 var (
@@ -63,9 +64,6 @@ func main() {
 	}
 
 	// Create new cert watcher
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel() // stops certwatcher
-
 	cw, err := webhook.NewCertWatcher(*certFile, *keyFile)
 	if err != nil {
 		klog.Fatalf("failed to initialize new cert watcher: %v", err)
@@ -75,6 +73,9 @@ func main() {
 	}
 
 	// Start the webhook server
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel() // stops certwatcher
+
 	if err := webhook.StartServer(ctx, tlsConfig, cw, *port); err != nil {
 		klog.Fatalf("server stopped: %v", err)
 	}

deploy/kubernetes/webhook-example/webhook.yaml

@@ -23,7 +23,6 @@ spec:
         args:
         - '--tls-cert-file=/etc/snapshot-conversion-webhook/certs/tls.crt'
         - '--tls-private-key-file=/etc/snapshot-conversion-webhook/certs/tls.key'
-        # uncomment the following line to enable webhook for VolumeGroupSnapshot, VolumeGroupSnapshotContent and VolumeGroupSnapshotClass.
         ports:
         - containerPort: 443 # change the port as needed
         volumeMounts:

pkg/sidecar-controller/groupsnapshot_helper.go

@@ -24,13 +24,13 @@ import (
 	"strings"
 	"time"
 
+	"github.com/container-storage-interface/spec/lib/go/csi"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/cache"
 	klog "k8s.io/klog/v2"
 
-	"github.com/container-storage-interface/spec/lib/go/csi"
 	crdv1beta2 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1beta2"
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
 	"github.com/kubernetes-csi/external-snapshotter/v8/pkg/utils"

pkg/webhook/convert.go

@@ -43,37 +43,26 @@ func convertGroupSnapshotCRD(obj *unstructured.Unstructured, toVersion string) (
 		return nil, statusErrorWithMessage("conversion from a version to itself should not call the webhook: %s", toVersion)
 	}
 
-	switch obj.GetAPIVersion() {
-	case "groupsnapshot.storage.k8s.io/v1beta1":
-		switch toVersion {
-		case "groupsnapshot.storage.k8s.io/v1beta2":
-			switch kind {
-			case "VolumeGroupSnapshotContent":
-				if err := convertVolumeGroupSnapshotContentFromV1beta1ToV1beta2(convertedObject); err != nil {
-					return nil, statusErrorWithMessage("%s", err.Error())
-				}
-			default:
-				return nil, statusErrorWithMessage("unexpected conversion kind %q", kind)
-			}
-		default:
-			return nil, statusErrorWithMessage("unexpected conversion version %q", toVersion)
+	if kind != "VolumeGroupSnapshotContent" {
+		return nil, statusErrorWithMessage("unexpected conversion kind %q", kind)
+	}
+
+	const v1beta1Version = "groupsnapshot.storage.k8s.io/v1beta1"
+	const v1beta2Version = "groupsnapshot.storage.k8s.io/v1beta2"
+
+	switch {
+	case fromVersion == v1beta1Version && toVersion == v1beta2Version:
+		if err := convertVolumeGroupSnapshotContentFromV1beta1ToV1beta2(convertedObject); err != nil {
+			return nil, statusErrorWithMessage("%s", err.Error())
 		}
-	case "groupsnapshot.storage.k8s.io/v1beta2":
-		switch toVersion {
-		case "groupsnapshot.storage.k8s.io/v1beta1":
-			switch kind {
-			case "VolumeGroupSnapshotContent":
-				if err := convertVolumeGroupSnapshotContentFromV1beta2ToV1beta1(convertedObject); err != nil {
-					return nil, statusErrorWithMessage("%s", err.Error())
-				}
-			default:
-				return nil, statusErrorWithMessage("unexpected conversion kind %q", kind)
-			}
-		default:
-			return nil, statusErrorWithMessage("unexpected conversion version %q", toVersion)
+
+	case fromVersion == v1beta2Version && toVersion == v1beta1Version:
+		if err := convertVolumeGroupSnapshotContentFromV1beta2ToV1beta1(convertedObject); err != nil {
+			return nil, statusErrorWithMessage("%s", err.Error())
 		}
+
 	default:
-		return nil, statusErrorWithMessage("unexpected conversion version %q", fromVersion)
+		return nil, statusErrorWithMessage("unexpected conversion version from %q to %q", fromVersion, toVersion)
 	}
 
 	return convertedObject, statusSucceed()
@@ -151,7 +140,7 @@ func convertVolumeGroupSnapshotContentFromV1beta2ToV1beta1(obj *unstructured.Uns
 				delete(mapEntry, "readyToUse")
 				delete(mapEntry, "restoreSize")
 			} else {
-				return fmt.Errorf("unexpected content in .status.volumeSnapshotInfoList[%q]: expected map", i)
+				return fmt.Errorf("unexpected content in .status.volumeSnapshotInfoList[%d]: expected map", i)
 			}
 		}
 

pkg/webhook/webhook_test.go

@@ -28,13 +28,14 @@ import (
 	"math/big"
 	"net"
 	"os"
+	"path"
 	"testing"
 	"time"
 )
 
 func TestWebhookCertReload(t *testing.T) {
 	// Initialize test space
-	tmpDir := os.TempDir() + "/webhook-cert-tests"
+	tmpDir := path.Join(t.TempDir(), "/webhook-cert-tests")
 	certFile := tmpDir + "/tls.crt"
 	keyFile := tmpDir + "/tls.key"
 	port := 30443