Repository: kubernetes-csi/external-snapshotter (go)

Issue:
Provide an option to pass snapshot metadata to csi driver
When external snapshotter sends a request to the csi-driver it doesn't contain any additional information about the snapshot name, snapshot namespace, or the snapshot content name, it would be nice if we can provide an option to do the same. currently the same feature is available in external-provisioner to pass the PVC metadata (PVC name, namespace, and PV name) as additional parameters in createvolume requests when `--extra-create-metadata`.

cc @xing-yang 

Let me know if it sounds good, I can work on it.

Pull request:
Pass snapshot metadata to CSI driver
@msau42 @xing-yang @Madhu-1 

<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**

/kind feature

**What this PR does / why we need it**:

This PR introduces an `--extra-create-metadata` flag to the CSI snapshotter.

When set to true, the VolumeSnapshot's name / namespace and the VolumeSnapshotContent's name will be passed as parameters to the CSI driver in `CreateSnapshotRequest`.

**Which issue(s) this PR fixes**:

Fixes #341

**Special notes for your reviewer**:

In the CSI provisioner we have a `csi.storage.k8s.io/` prefix defined for metadata passed as parameters to the CSI driver:

https://github.com/kubernetes-csi/external-provisioner/blob/b1d315e4b8dbc7e4a948ee2b9b3d7a58892cfa57/pkg/controller/controller.go#L108-L111

I want to use this same prefix in the CSI snapshotter for consistency, however in this repo this prefix is intended for parameters to remove.

https://github.com/kubernetes-csi/external-snapshotter/blob/60202d2e3149648af816b16525e51a457e66c292/pkg/utils/util.go#L49-L53

I modified the existing method that prunes these prefixed parameters to exclude these few. Any suggestions on refactoring to improve readability / code quality here are appreciated.

Outside of unit tests, I deployed this in a real cluster and observed the following creating a snapshot (taken from AWS EBS CSI driver logs):
```
I0914 21:30:40.192053       1 controller.go:412] CreateSnapshot: called with args source_volume_id:"vol-057e3e0fc815fe8dd" name:"snapshot-8f42c931-29f0-4755-8e3e-4f49ceb94dcf" parameters:<key:"csi.storage.k8s.io/volumesnapshot/name" value:"snapshot" > parameters:<key:"csi.storage.k8s.io/volumesnapshot/namespace" value:"default" > parameters:<key:"csi.storage.k8s.io/volumesnapshotcontent/name" value:"snapcontent-8f42c931-29f0-4755-8e3e-4f49ceb94dcf" >
```

**Does this PR introduce a user-facing change?**:
```release-note
The CSI snapshotter has a new `--extra-create-metadata` flag.

When set to true, the name and namespace of the source VolumeSnapshot, and the name of the source  VolumeSnapshotContent will be passed as parameters to the CSI driver in CreateSnapshotRequest.
```


Patch:
diff --git a/cmd/csi-snapshotter/main.go b/cmd/csi-snapshotter/main.go
index 8c606da21..97e831f3e 100644
--- a/cmd/csi-snapshotter/main.go
+++ b/cmd/csi-snapshotter/main.go
@@ -62,6 +62,7 @@ var (
 	showVersion            = flag.Bool("version", false, "Show version.")
 	threads                = flag.Int("worker-threads", 10, "Number of worker threads.")
 	csiTimeout             = flag.Duration("timeout", defaultCSITimeout, "The timeout for any RPCs to the CSI driver. Default is 1 minute.")
+	extraCreateMetadata    = flag.Bool("extra-create-metadata", false, "If set, add snapshot metadata to plugin snapshot requests as parameters.")
 
 	leaderElection          = flag.Bool("leader-election", false, "Enables leader election.")
 	leaderElectionNamespace = flag.String("leader-election-namespace", "", "The namespace where the leader election resource exists. Defaults to the pod namespace if not set.")
@@ -173,6 +174,7 @@ func main() {
 		*resyncPeriod,
 		*snapshotNamePrefix,
 		*snapshotNameUUIDLength,
+		*extraCreateMetadata,
 	)
 
 	run := func(context.Context) {
diff --git a/pkg/sidecar-controller/csi_handler.go b/pkg/sidecar-controller/csi_handler.go
index 5ff94ebf1..966282005 100644
--- a/pkg/sidecar-controller/csi_handler.go
+++ b/pkg/sidecar-controller/csi_handler.go
@@ -24,7 +24,6 @@ import (
 
 	crdv1 "github.com/kubernetes-csi/external-snapshotter/client/v3/apis/volumesnapshot/v1beta1"
 	"github.com/kubernetes-csi/external-snapshotter/v3/pkg/snapshotter"
-	"github.com/kubernetes-csi/external-snapshotter/v3/pkg/utils"
 )
 
 // Handler is responsible for handling VolumeSnapshot events from informer.
@@ -74,11 +73,7 @@ func (handler *csiHandler) CreateSnapshot(content *crdv1.VolumeSnapshotContent,
 	if err != nil {
 		return "", "", time.Time{}, 0, false, err
 	}
-	newParameters, err := utils.RemovePrefixedParameters(parameters)
-	if err != nil {
-		return "", "", time.Time{}, 0, false, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err)
-	}
-	return handler.snapshotter.CreateSnapshot(ctx, snapshotName, *content.Spec.Source.VolumeHandle, newParameters, snapshotterCredentials)
+	return handler.snapshotter.CreateSnapshot(ctx, snapshotName, *content.Spec.Source.VolumeHandle, parameters, snapshotterCredentials)
 }
 
 func (handler *csiHandler) DeleteSnapshot(content *crdv1.VolumeSnapshotContent, snapshotterCredentials map[string]string) error {
diff --git a/pkg/sidecar-controller/snapshot_controller.go b/pkg/sidecar-controller/snapshot_controller.go
index 8b0d51f52..51ad10415 100644
--- a/pkg/sidecar-controller/snapshot_controller.go
+++ b/pkg/sidecar-controller/snapshot_controller.go
@@ -284,7 +284,17 @@ func (ctrl *csiSnapshotSideCarController) createSnapshotWrapper(content *crdv1.V
 		return nil, fmt.Errorf("failed to add VolumeSnapshotBeingCreated annotation on the content %s: %q", content.Name, err)
 	}
 
-	driverName, snapshotID, creationTime, size, readyToUse, err := ctrl.handler.CreateSnapshot(content, class.Parameters, snapshotterCredentials)
+	parameters, err := utils.RemovePrefixedParameters(class.Parameters)
+	if err != nil {
+		return nil, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err)
+	}
+	if ctrl.extraCreateMetadata {
+		parameters[utils.PrefixedVolumeSnapshotNameKey] = content.Spec.VolumeSnapshotRef.Name
+		parameters[utils.PrefixedVolumeSnapshotNamespaceKey] = content.Spec.VolumeSnapshotRef.Namespace
+		parameters[utils.PrefixedVolumeSnapshotContentNameKey] = content.Name
+	}
+
+	driverName, snapshotID, creationTime, size, readyToUse, err := ctrl.handler.CreateSnapshot(content, parameters, snapshotterCredentials)
 	if err != nil {
 		// NOTE(xyang): handle create timeout
 		// If it is a final error, remove annotation to indicate
diff --git a/pkg/sidecar-controller/snapshot_controller_base.go b/pkg/sidecar-controller/snapshot_controller_base.go
index cb68d2e39..92a800579 100644
--- a/pkg/sidecar-controller/snapshot_controller_base.go
+++ b/pkg/sidecar-controller/snapshot_controller_base.go
@@ -40,11 +40,12 @@ import (
 )
 
 type csiSnapshotSideCarController struct {
-	clientset     clientset.Interface
-	client        kubernetes.Interface
-	driverName    string
-	eventRecorder record.EventRecorder
-	contentQueue  workqueue.RateLimitingInterface
+	clientset           clientset.Interface
+	client              kubernetes.Interface
+	driverName          string
+	eventRecorder       record.EventRecorder
+	contentQueue        workqueue.RateLimitingInterface
+	extraCreateMetadata bool
 
 	contentLister       storagelisters.VolumeSnapshotContentLister
 	contentListerSynced cache.InformerSynced
@@ -70,6 +71,7 @@ func NewCSISnapshotSideCarController(
 	resyncPeriod time.Duration,
 	snapshotNamePrefix string,
 	snapshotNameUUIDLength int,
+	extraCreateMetadata bool,
 ) *csiSnapshotSideCarController {
 	broadcaster := record.NewBroadcaster()
 	broadcaster.StartLogging(klog.Infof)
@@ -78,14 +80,15 @@ func NewCSISnapshotSideCarController(
 	eventRecorder = broadcaster.NewRecorder(scheme.Scheme, v1.EventSource{Component: fmt.Sprintf("csi-snapshotter %s", driverName)})
 
 	ctrl := &csiSnapshotSideCarController{
-		clientset:     clientset,
-		client:        client,
-		driverName:    driverName,
-		eventRecorder: eventRecorder,
-		handler:       NewCSIHandler(snapshotter, timeout, snapshotNamePrefix, snapshotNameUUIDLength),
-		resyncPeriod:  resyncPeriod,
-		contentStore:  cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc),
-		contentQueue:  workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "csi-snapshotter-content"),
+		clientset:           clientset,
+		client:              client,
+		driverName:          driverName,
+		eventRecorder:       eventRecorder,
+		handler:             NewCSIHandler(snapshotter, timeout, snapshotNamePrefix, snapshotNameUUIDLength),
+		resyncPeriod:        resyncPeriod,
+		contentStore:        cache.NewStore(cache.DeletionHandlingMetaNamespaceKeyFunc),
+		contentQueue:        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "csi-snapshotter-content"),
+		extraCreateMetadata: extraCreateMetadata,
 	}
 
 	volumeSnapshotContentInformer.Informer().AddEventHandlerWithResyncPeriod(
diff --git a/pkg/utils/util.go b/pkg/utils/util.go
index c7c52086a..c47448553 100644
--- a/pkg/utils/util.go
+++ b/pkg/utils/util.go
@@ -49,7 +49,9 @@ const (
 	// CSI Parameters prefixed with csiParameterPrefix are not passed through
 	// to the driver on CreateSnapshotRequest calls. Instead they are intended
 	// to be used by the CSI external-snapshotter and maybe used to populate
-	// fields in subsequent CSI calls or Kubernetes API objects.
+	// fields in subsequent CSI calls or Kubernetes API objects. An exception
+	// exists for the volume snapshot and volume snapshot content keys, which are
+	// passed as parameters on CreateSnapshotRequest calls.
 	csiParameterPrefix = "csi.storage.k8s.io/"
 
 	PrefixedSnapshotterSecretNameKey      = csiParameterPrefix + "snapshotter-secret-name"      // Prefixed name key for DeleteSnapshot secret
@@ -58,6 +60,10 @@ const (
 	PrefixedSnapshotterListSecretNameKey      = csiParameterPrefix + "snapshotter-list-secret-name"      // Prefixed name key for ListSnapshots secret
 	PrefixedSnapshotterListSecretNamespaceKey = csiParameterPrefix + "snapshotter-list-secret-namespace" // Prefixed namespace key for ListSnapshots secret
 
+	PrefixedVolumeSnapshotNameKey        = csiParameterPrefix + "volumesnapshot/name"        // Prefixed VolumeSnapshot name key
+	PrefixedVolumeSnapshotNamespaceKey   = csiParameterPrefix + "volumesnapshot/namespace"   // Prefixed VolumeSnapshot namespace key
+	PrefixedVolumeSnapshotContentNameKey = csiParameterPrefix + "volumesnapshotcontent/name" // Prefixed VolumeSnapshotContent name key
+
 	// Name of finalizer on VolumeSnapshotContents that are bound by VolumeSnapshots
 	VolumeSnapshotContentFinalizer = "snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection"
 	// Name of finalizer on VolumeSnapshot that is being used as a source to create a PVC


Test patch:
diff --git a/pkg/sidecar-controller/content_create_test.go b/pkg/sidecar-controller/content_create_test.go
index 9e1aa04b2..9c0b066bf 100644
--- a/pkg/sidecar-controller/content_create_test.go
+++ b/pkg/sidecar-controller/content_create_test.go
@@ -41,6 +41,11 @@ func TestSyncContent(t *testing.T) {
 					snapshotName: "snapshot-snapuid1-1",
 					driverName:   mockDriverName,
 					snapshotId:   "snapuid1-1",
+					parameters: map[string]string{
+						utils.PrefixedVolumeSnapshotNameKey:        "snap1-1",
+						utils.PrefixedVolumeSnapshotNamespaceKey:   "default",
+						utils.PrefixedVolumeSnapshotContentNameKey: "content1-1",
+					},
 					creationTime: timeNow,
 					readyToUse:   true,
 				},
@@ -63,6 +68,11 @@ func TestSyncContent(t *testing.T) {
 					snapshotName: "snapshot-snapuid1-2",
 					driverName:   mockDriverName,
 					snapshotId:   "snapuid1-2",
+					parameters: map[string]string{
+						utils.PrefixedVolumeSnapshotNameKey:        "snap1-2",
+						utils.PrefixedVolumeSnapshotNamespaceKey:   "default",
+						utils.PrefixedVolumeSnapshotContentNameKey: "content1-2",
+					},
 					creationTime: timeNow,
 					readyToUse:   true,
 					size:         defaultSize,
@@ -114,7 +124,13 @@ func TestSyncContent(t *testing.T) {
 				{
 					volumeHandle: "volume-handle-1-4",
 					snapshotName: "snapshot-snapuid1-4",
-					parameters:   class5Parameters,
+					parameters: map[string]string{
+						utils.AnnDeletionSecretRefName:             "secret",
+						utils.AnnDeletionSecretRefNamespace:        "default",
+						utils.PrefixedVolumeSnapshotNameKey:        "snap1-4",
+						utils.PrefixedVolumeSnapshotNamespaceKey:   "default",
+						utils.PrefixedVolumeSnapshotContentNameKey: "content1-4",
+					},
 					secrets: map[string]string{
 						"foo": "bar",
 					},
diff --git a/pkg/sidecar-controller/framework_test.go b/pkg/sidecar-controller/framework_test.go
index 675d24bfe..652c15a27 100644
--- a/pkg/sidecar-controller/framework_test.go
+++ b/pkg/sidecar-controller/framework_test.go
@@ -521,6 +521,7 @@ func newTestController(kubeClient kubernetes.Interface, clientset clientset.Inte
 		60*time.Second,
 		"snapshot",
 		-1,
+		true,
 	)
 
 	ctrl.eventRecorder = record.NewFakeRecorder(1000)
diff --git a/pkg/utils/util_test.go b/pkg/utils/util_test.go
index 148f62a48..a25d456a4 100644
--- a/pkg/utils/util_test.go
+++ b/pkg/utils/util_test.go
@@ -169,8 +169,10 @@ func TestRemovePrefixedCSIParams(t *testing.T) {
 		{
 			name: "all known prefixed",
 			params: map[string]string{
-				PrefixedSnapshotterSecretNameKey:      "csiBar",
-				PrefixedSnapshotterSecretNamespaceKey: "csiBar",
+				PrefixedSnapshotterSecretNameKey:          "csiBar",
+				PrefixedSnapshotterSecretNamespaceKey:     "csiBar",
+				PrefixedSnapshotterListSecretNameKey:      "csiBar",
+				PrefixedSnapshotterListSecretNamespaceKey: "csiBar",
 			},
 			expectedParams: map[string]string{},
 		},