KEP-3141: Prevent unauthorised volume mode conversion #3151
Conversation
RaunakShah
commented
Jan 17, 2022
- One-line PR description: Prevent unauthorised volume mode conversion when restoring volumes.
- Issue link: Prevent unauthorised volume mode conversion during volume restore #3141
- Other comments:
|
/assign @jsafrane @msau42 @jingxu97 @yuxiangqian |
|
/assign @deads2k |
|
/assign @liggitt |
| ###### Are there any tests for feature enablement/disablement? | ||
|
|
||
| We will add unit tests with and without the feature gate enabled. The expectation | ||
| is for new fields in `VolumeSnapshotContent` to be dropped when the feature gate |
There was a problem hiding this comment.
I'm not sure if we should "drop" the new field as we don't have a real feature gate. Maybe we should just ignore the new field if the feature flag is disabled. You can keep this as is. I'll let others comment on this.
|
/lgtm |
|
/assign @enj |
|
/lgtm |
| API spec as well as the control flows of `snapshot-controller` and `external-provisioner`. | ||
| `VolumeSnapshotContent` API will include a field that denotes the volume mode of | ||
| the volume that the snapshot was created from. | ||
| This proposal also introduces a new annotation on the `VolumeSnapshotContent` resource |
There was a problem hiding this comment.
if we own the API, why make this an annotation?
There was a problem hiding this comment.
We did think about adding this to API as well, but thought an annotation is simpler. I'm open to adding this to API. So we can add allowVolumeModeChange to VolumeSnapshotClass for dynamic provisioning. This field will be copied to allowVolumeModeChange field in VolumeSnapshotContent by the snapshot-controller. For static provisioning, admin needs to set the allowVolumeModeChange field in VolumeSnapshotContent.
@jsafrane what do you think?
| logs or events for this purpose. | ||
| --> | ||
|
|
||
| ###### How can someone using this feature know that it is working for their instance? |
There was a problem hiding this comment.
At what stage will the PVC fail? This may be a challenge to give good feedback when you move to beta. I suggest thinking about this for your alpha implementation.
There was a problem hiding this comment.
If the new feature flag is enabled, external-provisioner will be checking whether allowVolumeModeChange is set to true if the volume mode is changed when creating a PVC from a VolumeSnapshot. If allowVolumeModeChange is set to false, CreateVolume request won't be sent to the CSI driver and a PV won't be created. An event should be reported on PVC indicating "ProvisioningFailed". User can also check external-provisioner logs to find out why the volume is not created.
There was a problem hiding this comment.
User can also check external-provisioner logs to find out why the volume is not created.
Pod creating users cannot see this log.
There was a problem hiding this comment.
That's true. They can check events on PVC. We'll need to think more about the troubleshooting part.
|
I suspect the API will get some comments during review about making it a field. I encourage you to spend time on the monitoring and troubleshooting flows during alpha. When transitioning to beta, those areas face scrutiny in PRR. This PRR is sufficient for alpha. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, jsafrane, RaunakShah The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |