Repository: kubernetes-csi/external-snapshotter
Pull request #679 (2022-03-31 06:13:32)

Changes to snapshot controller to add sourceVolumeMode
<!--  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 adds changes to snapshot-controller to read the volume mode of the source PV and add it to the `Spec.SourceVolumeMode` field of the to-be-created `VolumeSnapshotContent` object. 

Also introduce a feature flag for this feature. 

KEP - https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/3141-prevent-volume-mode-conversion


Testing

1. Deploy Hostpath CSI driver with `v6.0.0-rc2` version of `csi-snapshotter` and `VolumeSnapshotContent` CRD. Set `prevent-volume-mode-conversion` to `true` in `snapshot-controller`. 
```
% kubectl get deployment -n kube-system -o yaml snapshot-controller
apiVersion: apps/v1
kind: Deployment
...
spec:
  minReadySeconds: 15
  progressDeadlineSeconds: 600
...
spec:
      containers:
      - args:
        - --v=5
        - --leader-election=true
        - --prevent-volume-mode-conversion=true
        image: snapshot-controller:latest
...
```

2. Create a VolumeSnapshot object from an existing PVC:

```
% kubectl get volumesnapshot -A
NAMESPACE   NAME                   READYTOUSE   SOURCEPVC   SOURCESNAPSHOTCONTENT   RESTORESIZE   SNAPSHOTCLASS               SNAPSHOTCONTENT                                    CREATIONTIME   AGE
default     new-snapshot-demo-v1   true         hpvc                                1Gi           csi-hostpath-snapclass-v1   snapcontent-f4161ad9-9279-4e77-93ee-f8b5898b6010   4d             4d
```

3. Verify VolumeSnapshotContent object for SourceVolumeMode field:
```
% kubectl get volumesnapshotcontent -o yaml
apiVersion: v1
items:
- apiVersion: snapshot.storage.k8s.io/v1
  kind: VolumeSnapshotContent
...
  spec:
    deletionPolicy: Delete
    driver: hostpath.csi.k8s.io
    source:
      volumeHandle: 37a4f46e-afe9-11ec-b141-8ed1e89c66d6
    sourceVolumeMode: Filesystem
    volumeSnapshotClassName: csi-hostpath-snapclass-v1
...
```

4. Verify logs of `snapshot-controller`:

```
I0404 08:10:53.813378       1 snapshot_controller.go:698] snapcontent snapcontent-bbf7be38-ccb9-46bf-a420-f1a436bf10e4 has volume mode Filesystem
```

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Changes to snapshot controller to add SourceVolumeMode to VolumeSnapshotContents
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-snapshotter/pull/679#" title="Author self-approved">RaunakShah</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **jsafrane** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @jsafrane` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-snapshotter).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-snapshotter/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["jsafrane"]} -->
k8s-ci-robot: @RaunakShah: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-kubernetes-csi-external-snapshotter-unit | 886f9c5b61bb42ed2eaa09753f26d1c233ae74e5 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-snapshotter/679/pull-kubernetes-csi-external-snapshotter-unit/1509413572076113920) | true | `/test pull-kubernetes-csi-external-snapshotter-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-snapshotter&pr=679). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3ARaunakShah). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-snapshotter/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
RaunakShah: /hold until https://github.com/kubernetes-csi/external-snapshotter/pull/683 is merged


RaunakShah: /hold until https://github.com/kubernetes-csi/external-snapshotter/pull/683 is merged



Patch:
diff --git a/cmd/snapshot-controller/main.go b/cmd/snapshot-controller/main.go
index 0ff184986..1c91bfc6d 100644
--- a/cmd/snapshot-controller/main.go
+++ b/cmd/snapshot-controller/main.go
@@ -70,6 +70,7 @@ var (
 	retryIntervalStart            = flag.Duration("retry-interval-start", time.Second, "Initial retry interval of failed volume snapshot creation or deletion. It doubles with each failure, up to retry-interval-max. Default is 1 second.")
 	retryIntervalMax              = flag.Duration("retry-interval-max", 5*time.Minute, "Maximum retry interval of failed volume snapshot creation or deletion. Default is 5 minutes.")
 	enableDistributedSnapshotting = flag.Bool("enable-distributed-snapshotting", false, "Enables each node to handle snapshotting for the local volumes created on that node")
+	preventVolumeModeConversion   = flag.Bool("prevent-volume-mode-conversion", false, "Prevents an unauthorised user from modifying the volume mode when creating a PVC from an existing VolumeSnapshot.")
 )
 
 var (
@@ -187,6 +188,7 @@ func main() {
 		workqueue.NewItemExponentialFailureRateLimiter(*retryIntervalStart, *retryIntervalMax),
 		workqueue.NewItemExponentialFailureRateLimiter(*retryIntervalStart, *retryIntervalMax),
 		*enableDistributedSnapshotting,
+		*preventVolumeModeConversion,
 	)
 
 	if err := ensureCustomResourceDefinitionsExist(snapClient); err != nil {
diff --git a/pkg/common-controller/snapshot_controller.go b/pkg/common-controller/snapshot_controller.go
index 4c8dd0262..2315c1306 100644
--- a/pkg/common-controller/snapshot_controller.go
+++ b/pkg/common-controller/snapshot_controller.go
@@ -684,6 +684,13 @@ func (ctrl *csiSnapshotCommonController) createSnapshotContent(snapshot *crdv1.V
 		}
 	}
 
+	if ctrl.preventVolumeModeConversion {
+		if volume.Spec.VolumeMode != nil {
+			snapshotContent.Spec.SourceVolumeMode = volume.Spec.VolumeMode
+			klog.V(5).Infof("snapcontent %s has volume mode %s", snapshotContent.Name, *snapshotContent.Spec.SourceVolumeMode)
+		}
+	}
+
 	// Set AnnDeletionSecretRefName and AnnDeletionSecretRefNamespace
 	if snapshotterSecretRef != nil {
 		klog.V(5).Infof("createSnapshotContent: set annotation [%s] on content [%s].", utils.AnnDeletionSecretRefName, snapshotContent.Name)
diff --git a/pkg/common-controller/snapshot_controller_base.go b/pkg/common-controller/snapshot_controller_base.go
index 7ff128a9c..c53fd7374 100644
--- a/pkg/common-controller/snapshot_controller_base.go
+++ b/pkg/common-controller/snapshot_controller_base.go
@@ -68,6 +68,7 @@ type csiSnapshotCommonController struct {
 	resyncPeriod time.Duration
 
 	enableDistributedSnapshotting bool
+	preventVolumeModeConversion   bool
 }
 
 // NewCSISnapshotController returns a new *csiSnapshotCommonController
@@ -84,6 +85,7 @@ func NewCSISnapshotCommonController(
 	snapshotRateLimiter workqueue.RateLimiter,
 	contentRateLimiter workqueue.RateLimiter,
 	enableDistributedSnapshotting bool,
+	preventVolumeModeConversion bool,
 ) *csiSnapshotCommonController {
 	broadcaster := record.NewBroadcaster()
 	broadcaster.StartLogging(klog.Infof)
@@ -138,6 +140,8 @@ func NewCSISnapshotCommonController(
 		ctrl.nodeListerSynced = nodeInformer.Informer().HasSynced
 	}
 
+	ctrl.preventVolumeModeConversion = preventVolumeModeConversion
+
 	return ctrl
 }
 


Test patch:
diff --git a/pkg/common-controller/framework_test.go b/pkg/common-controller/framework_test.go
index ab86cb92e..dd05977c7 100644
--- a/pkg/common-controller/framework_test.go
+++ b/pkg/common-controller/framework_test.go
@@ -844,6 +844,7 @@ func newTestController(kubeClient kubernetes.Interface, clientset clientset.Inte
 		workqueue.NewItemExponentialFailureRateLimiter(1*time.Millisecond, 1*time.Minute),
 		workqueue.NewItemExponentialFailureRateLimiter(1*time.Millisecond, 1*time.Minute),
 		false,
+		false,
 	)
 
 	ctrl.eventRecorder = record.NewFakeRecorder(1000)