Conversion webhook review

This patch makes several corrections to the conversion webhook:

- fixes several occurrences of the old validation webhook codebase
- simplify the webhook command and drops the direct cobra dependency
- align the README with what the code really does
- removes the unneeded structure in the unit tests
- removes unneeded code and the unneeded dependencies
- drop support for converting VolumeSnapshots and VolumeSnapshotClasses as they
  are structurally identical

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

Author: leonardoce

client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotclasses.yaml

@@ -17,15 +17,6 @@ spec:
     - vgsclasses
     singular: volumegroupsnapshotclass
   scope: Cluster
-  conversion:
-    strategy: Webhook
-    webhook:
-      conversionReviewVersions: ["v1","v1beta1"]
-      clientConfig:
-        service:
-          namespace: default
-          name: snapshot-conversion-webhook-service
-          path: /convert
   versions:
   - additionalPrinterColumns:
     - jsonPath: .driver

client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshots.yaml

@@ -16,15 +16,6 @@ spec:
     - vgs
     singular: volumegroupsnapshot
   scope: Namespaced
-  conversion:
-    strategy: Webhook
-    webhook:
-      conversionReviewVersions: ["v1","v1beta1"]
-      clientConfig:
-        service:
-          namespace: default
-          name: snapshot-conversion-webhook-service
-          path: /convert
   versions:
   - additionalPrinterColumns:
     - description: Indicates if all the individual snapshots in the group are ready

cmd/snapshot-conversion-webhook/main.go

@@ -17,17 +17,65 @@ limitations under the License.
 package main
 
 import (
+	"context"
+	"crypto/tls"
 	"flag"
 
-	webhook "github.com/kubernetes-csi/external-snapshotter/v8/pkg/webhook"
+	"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"
 )
 
+var (
+	certFile = flag.String(
+		"tls-cert-file",
+		"",
+		"File containing the x509 Certificate for HTTPS. (CA cert, if any, concatenated after server cert). Required.",
+	)
+	keyFile = flag.String(
+		"tls-private-key-file",
+		"",
+		"File containing the x509 private key matching --tls-cert-file. Required.",
+	)
+	port = flag.Int(
+		"port",
+		443,
+		"Secure port that the webhook listens on",
+	)
+)
+
 func main() {
-	rootCmd := webhook.CmdWebhook
+	c := logsapi.NewLoggingConfiguration()
+	logsapi.AddGoFlags(c, flag.CommandLine)
+	logs.InitLogs()
+	standardflags.AddAutomaxprocs(klog.Infof)
+	flag.Parse()
+
+	klog.Info("Starting conversion webhook server")
+
+	if certFile == nil || *certFile == "" {
+		klog.Fatal("--tls-cert-file must be specified")
+	}
+	if keyFile == nil || *keyFile == "" {
+		klog.Fatal("--tls-private-key-file must be specified")
+	}
+
+	// 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)
+	}
+	tlsConfig := &tls.Config{
+		GetCertificate: cw.GetCertificate,
+	}
 
-	loggingFlags := &flag.FlagSet{}
-	klog.InitFlags(loggingFlags)
-	rootCmd.PersistentFlags().AddGoFlagSet(loggingFlags)
-	rootCmd.Execute()
+	// Start the webhook server
+	if err := webhook.StartServer(ctx, tlsConfig, cw, *port); err != nil {
+		klog.Fatalf("server stopped: %v", err)
+	}
 }

deploy/kubernetes/webhook-example/README.md

@@ -1,6 +1,11 @@
-# Validating Webhook
+# Conversion Webhook
 
-The snapshot validating webhook is an HTTP callback which responds to [admission requests](https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/). It is part of a larger [plan](https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1900-volume-snapshot-validation-webhook#proposal) to tighten validation for volume snapshot objects. This webhook introduces the [ratcheting validation](https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1900-volume-snapshot-validation-webhook#backwards-compatibility) mechanism targeting the tighter validation. The cluster admin or Kubernetes distribution admin should install the webhook alongside the snapshot controllers and CRDs.
+The snapshot conversion webhook is an HTTP callback which responds to 
+[conversion requests](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#webhook-conversion),
+allowing the API server to convert between the VolumeGroupSnapshotContent v1beta1 API to and from the v1beta2 API.
+
+The cluster admin or Kubernetes distribution admin should install the webhook
+alongside the snapshot controllers and CRDs.
 
 ## How to build the webhook
 
@@ -18,11 +23,25 @@ docker build -t snapshot-conversion-webhook:latest -f ./cmd/snapshot-conversion-
 
 ## How to deploy the webhook
 
-The webhook server is provided as an image which can be built from this repository. It can be deployed anywhere, as long as the api server is able to reach it over HTTPS. It is recommended to deploy the webhook server in the cluster as snapshotting is latency sensitive. A `ValidatingWebhookConfiguration` object is needed to configure the api server to contact the webhook server. Please see the [documentation](https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/) for more details. The webhook server code is adapted from the [webhook server](https://github.com/kubernetes/kubernetes/tree/v1.18.6/test/images/agnhost/webhook) used in the kubernetes/kubernetes end to end testing code.
+The webhook server is provided as an image which can be built from this repository. It can be deployed anywhere, 
+as long as the api server is able to reach it over HTTPS. It is recommended to deploy the webhook server in the
+cluster as snapshotting is latency sensitive.
+
+The CRD may need to be patched to allow safe TLS communication to the webhook server. 
+Please see the [documentation](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#webhook-conversion)
+for more details.
+
+The webhook server code is adapted from the [webhook server](https://github.com/kubernetes/kubernetes/blob/v1.25.3/test/images/agnhost/crd-conversion-webhook/main.go)
+used in the kubernetes/kubernetes e2e testing code.
 
 ### Example in-cluster deployment using Kubernetes Secrets
 
-Please note this is not considered to be a production ready method to deploy the certificates and is only provided for demo purposes. This is only one of many ways to deploy the certificates, it is your responsibility to ensure the security of your cluster. TLS certificates and private keys should be handled with care and you may not want to keep them in plain Kubernetes secrets.
+Please note this is not considered to be a production ready method to deploy the certificates and is only provided
+for demo purposes. This is only one of many ways to deploy the certificates, it is your responsibility to
+ensure the security of your cluster.
+
+TLS certificates and private keys should be handled with care and you may not want to keep them in plain
+Kubernetes secrets.
 
 This method was heavily adapted from [banzai cloud](https://banzaicloud.com/blog/k8s-admission-webhooks/).
 
@@ -38,7 +57,7 @@ These commands should be run from the top level directory.
     ./deploy/kubernetes/webhook-example/create-cert.sh --service snapshot-conversion-webhook-service --secret snapshot-conversion-webhook-secret --namespace default # Make sure to use a different namespace
     ```
 
-2. Patch the VolumeGroupSnapshot, VolumeGroupSnapshotContent and VolumeGroupSnapshotClass CRDs filling in the CA bundle field.
+2. Patch the VolumeGroupSnapshotContent CRD filling in the CA bundle field.
 
     ```bash
     ./deploy/kubernetes/webhook-example/patch-ca-bundle.sh
@@ -56,20 +75,18 @@ Once all the pods from the deployment are up and running, you should be ready to
 
 #### Verify the webhook works
 
-Try to query the API server for a VolumeGroupSnapshot object in the version `v1beta1`.
+Try to query the API server for a VolumeGroupSnapshotContent object in the version `v1beta1`.
 
 ```bash
-kubectl get volumegroupsnapshotclass.v1beta1.groupsnapshot.storage.k8s.io 
-
 kubectl get volumegroupsnapshotcontent.v1beta1.groupsnapshot.storage.k8s.io 
-
-kubectl get volumegroupsnapshot.v1beta1.groupsnapshot.storage.k8s.io 
 ```
 
 ### Other methods to deploy the webhook server
 
-Look into [cert-manager](https://cert-manager.io/) to handle the certificates, and this kube-builder [tutorial](https://book.kubebuilder.io/cronjob-tutorial/cert-manager.html) on how to deploy a webhook.
+Look into [cert-manager](https://cert-manager.io/) to handle the certificates,
+and this kube-builder [tutorial](https://book.kubebuilder.io/cronjob-tutorial/cert-manager.html) on how to deploy a webhook.
 
 #### Important
 
-Please see the deployment [yaml](./webhook.yaml) for the arguments expected by the webhook server. The snapshot validation webhook is served at the path `/volumesnapshot`.
+Please see the deployment [yaml](./webhook.yaml) for the arguments expected by the
+webhook server. The conversion webhook is served at the path `/convert`.

deploy/kubernetes/webhook-example/webhook.yaml

@@ -18,7 +18,7 @@ spec:
     spec:
       containers:
       - name: snapshot-conversion-webhook
-        image: registry.k8s.io/sig-storage/snapshot-conversion-webhook:v8.0.1 # change the image if you wish to use your own custom validation server image
+        image: registry.k8s.io/sig-storage/snapshot-conversion-webhook:v8.0.1 # change the image if you wish to use your own custom conversion server image
         imagePullPolicy: IfNotPresent
         args:
         - '--tls-cert-file=/etc/snapshot-conversion-webhook/certs/tls.crt'

go.mod

@@ -9,15 +9,13 @@ require (
 	github.com/evanphx/json-patch v5.9.0+incompatible
 	github.com/fsnotify/fsnotify v1.7.0
 	github.com/golang/mock v1.6.0
-	github.com/google/gofuzz v1.2.0
 	github.com/kubernetes-csi/csi-lib-utils v0.22.0
 	github.com/kubernetes-csi/csi-test/v5 v5.3.1
 	github.com/kubernetes-csi/external-snapshotter/client/v8 v8.2.0
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822
 	github.com/prometheus/client_golang v1.22.0
 	github.com/prometheus/client_model v0.6.1
 	github.com/prometheus/common v0.62.0
-	github.com/spf13/cobra v1.8.1
 	google.golang.org/grpc v1.69.0
 	google.golang.org/protobuf v1.36.5
 	k8s.io/api v0.33.2
@@ -57,6 +55,7 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
 	github.com/prometheus/procfs v0.15.1 // indirect
+	github.com/spf13/cobra v1.8.1 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
 	github.com/x448/float16 v0.8.4 // indirect
 	go.opentelemetry.io/auto/sdk v1.1.0 // indirect

go.sum

@@ -47,8 +47,6 @@ github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeN
 github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
 github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
-github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0=
-github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/pprof v0.0.0-20241029153458-d1b30febd7db h1:097atOisP2aRj7vFgYQBbFN4U4JNXUNYpxael3UzMyo=
 github.com/google/pprof v0.0.0-20241029153458-d1b30febd7db/go.mod h1:vavhavw2zAxS5dIdcRluK6cSGGPlZynqzFM8NdvU144=
 github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=

pkg/webhook/convert.go

@@ -48,10 +48,8 @@ func convertGroupSnapshotCRD(obj *unstructured.Unstructured, toVersion string) (
 		switch toVersion {
 		case "groupsnapshot.storage.k8s.io/v1beta2":
 			switch kind {
-			case "VolumeGroupSnapshot":
-			case "VolumeGroupSnapshotClass":
 			case "VolumeGroupSnapshotContent":
-				if err := convertVolumeGroupSnapshotFromV1beta1ToV1beta2(convertedObject); err != nil {
+				if err := convertVolumeGroupSnapshotContentFromV1beta1ToV1beta2(convertedObject); err != nil {
 					return nil, statusErrorWithMessage("%s", err.Error())
 				}
 			default:
@@ -64,10 +62,8 @@ func convertGroupSnapshotCRD(obj *unstructured.Unstructured, toVersion string) (
 		switch toVersion {
 		case "groupsnapshot.storage.k8s.io/v1beta1":
 			switch kind {
-			case "VolumeGroupSnapshot":
-			case "VolumeGroupSnapshotClass":
 			case "VolumeGroupSnapshotContent":
-				if err := convertVolumeGroupSnapshotFromV1beta2ToV1beta1(convertedObject); err != nil {
+				if err := convertVolumeGroupSnapshotContentFromV1beta2ToV1beta1(convertedObject); err != nil {
 					return nil, statusErrorWithMessage("%s", err.Error())
 				}
 			default:
@@ -83,7 +79,7 @@ func convertGroupSnapshotCRD(obj *unstructured.Unstructured, toVersion string) (
 	return convertedObject, statusSucceed()
 }
 
-func convertVolumeGroupSnapshotFromV1beta1ToV1beta2(obj *unstructured.Unstructured) error {
+func convertVolumeGroupSnapshotContentFromV1beta1ToV1beta2(obj *unstructured.Unstructured) error {
 	annotations := obj.GetAnnotations()
 	if value, ok := annotations[volumeSnapshotInfoAnnotationName]; ok {
 		// We use the annotation to fill the missing fields into the status
@@ -129,7 +125,7 @@ func convertVolumeGroupSnapshotFromV1beta1ToV1beta2(obj *unstructured.Unstructur
 	return nil
 }
 
-func convertVolumeGroupSnapshotFromV1beta2ToV1beta1(obj *unstructured.Unstructured) error {
+func convertVolumeGroupSnapshotContentFromV1beta2ToV1beta1(obj *unstructured.Unstructured) error {
 	volumeSnapshotInfoList, found, err := unstructured.NestedSlice(obj.Object, "status", "volumeSnapshotInfoList")
 	if err != nil {
 		return fmt.Errorf("unable to traverse for .status.volumeSnapshotInfoList: %w", err)

pkg/webhook/convert_test.go

@@ -39,7 +39,7 @@ func TestFromBeta1ToBeta2(t *testing.T) {
 			from := fromFile(t, beta1FileName)
 			to := fromFile(t, beta2FileName)
 
-			err := convertVolumeGroupSnapshotFromV1beta1ToV1beta2(from)
+			err := convertVolumeGroupSnapshotContentFromV1beta1ToV1beta2(from)
 			if err != nil {
 				t.Fatalf("conversion failed: %v", err.Error())
 			}
@@ -65,7 +65,7 @@ func TestFromBeta2ToBeta1(t *testing.T) {
 			from := fromFile(t, beta2FileName)
 			to := fromFile(t, beta1FileName)
 
-			err := convertVolumeGroupSnapshotFromV1beta2ToV1beta1(from)
+			err := convertVolumeGroupSnapshotContentFromV1beta2ToV1beta1(from)
 			if err != nil {
 				t.Fatalf("conversion failed: %v", err.Error())
 			}