Should set snapshot.storage.kubernetes.io/deletion-secret-name(space) annotation automatically to VolumeSnapshotContent when importing an exists snapshot

[bells17]

Hi, I think it should set snapshot.storage.kubernetes.io/deletion-secret-name(space) annotation automatically to VolumeSnapshotContent when importing an exists snapshot.

Now, If your CSI driver uses VolumeSnapshotClass Secrets in DeleteSnapshot RPC, may DeleteSnapshot is failed.
Because the VolumeSnapshotClass Secrets don't be used when requesting the DeleteSnapshot RPC call.

If you want to use VolumeSnapshotClass Secrets in DeleteSnapshot RPC, You need to set snapshot.storage.kubernetes.io/deletion-secret-name(space) annotation manually to VolumeSnapshotContent.

But if VolumeSnapshotContent is created from VolumeSnapshot, snapshot.storage.kubernetes.io/deletion-secret-name(space) annotation annotation set automatically to VolumeSnapshotContent.

So I think it should set snapshot.storage.kubernetes.io/deletion-secret-name(space) annotation automatically to VolumeSnapshotContent when importing an exists snapshot.

If annotations are set automatically, users don't need to think about annotations when importing an existing snapshot.

[xing-yang]

Yes, it makes sense to set deletion secret annotation for pre-provisioned snapshots as well.

I think it should be added in this routine and secrets should be retrieved from snapshotClass on the content for pre-provisioned case (same as ListSnapshots secret):
https://github.com/kubernetes-csi/external-snapshotter/blob/master/pkg/sidecar-controller/snapshot_controller.go#L261

One thing we need to decide is how to handle the case where annotation is already applied. For example, a snapshot was dynamically provisioned and secret annotation was added to the content. This content has Retain policy. User deletes VolumeSnapshot. Content remains. User changes the snapshotClassName or secrets in snapshotClass associated with the content. User creates a new VolumeSnapshot to bind with the existing content. At this time, should we keep the original annotation or modify it with the new one.

[xing-yang]

CC @msau42

[msau42]

In terms of responsibility, I think we currently have some clear boundaries:

• When dynamically provisioning a snapshot, it's the snapshot controller that creates and owns the SnapshotContent object. The only thing the controller can modify is the Status.
• When manually importing a snapshot, it's the admin that creates and owns the object.

I don't think we should mix responsibilities and have the controller modify the Content object.

This is the same behavior for PVs. If someone manually creates a PV object, they need to manually fill in all the fields.

[humblec]

I agree with @msau42 here, with that, this is no longer an issue or we dont have to fix something here. Can we close this @bells17 ?

[humblec]

/assign @humblec

[bells17]

Thank you for response @xing-yang @msau42.
I understand.

@humblec Yes, I closed this issue. Thank you for notification.

[humblec]

Thanks @bells17 👍