From 8bbd546167cdb6fcd8fe6a9eaa9d6718ab0f2e64 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Fri, 21 Aug 2026 11:19:57 +0800 Subject: [PATCH] Double check the label for backup when deleting VSC (#10346) This commit double checks the label of the VSC on the cluster before deleting it to avoid mis-deletion. Signed-off-by: Daniel Jiang --- changelogs/unreleased/10346-reasonerjt | 1 + .../csi/volumesnapshotcontent_action.go | 14 ++- .../csi/volumesnapshotcontent_action_test.go | 107 ++++++++++++++---- 3 files changed, 100 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/10346-reasonerjt diff --git a/changelogs/unreleased/10346-reasonerjt b/changelogs/unreleased/10346-reasonerjt new file mode 100644 index 000000000..eab333072 --- /dev/null +++ b/changelogs/unreleased/10346-reasonerjt @@ -0,0 +1 @@ +Double check the label for backup when deleting VSC- #10346 diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action.go b/internal/delete/actions/csi/volumesnapshotcontent_action.go index c57a0eb1a..4b7895341 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action.go @@ -86,7 +86,7 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute( // This handles legacy (pre-1.15) backups where the original VSC // with DeletionPolicy=Retain still exists in the cluster. originalVSCName := snapCont.Name - if cleaned := p.tryDeleteOriginalVSC(context.TODO(), originalVSCName); cleaned { + if cleaned := p.tryDeleteOriginalVSC(context.TODO(), originalVSCName, input.Backup.Name); cleaned { p.log.Infof("Successfully deleted original VolumeSnapshotContent %s from cluster, skipping temp VSC creation", originalVSCName) return nil } @@ -149,10 +149,11 @@ func (p *volumeSnapshotContentDeleteItemAction) Execute( // the cluster (legacy pre-1.15 backups). It patches the DeletionPolicy to // Delete so the CSI driver also removes the cloud snapshot, then deletes // the VSC object itself. -// Returns true if the original VSC was found and deletion was initiated. +// Returns true if the original VSC was found, carries the backup label, and deletion was initiated. func (p *volumeSnapshotContentDeleteItemAction) tryDeleteOriginalVSC( ctx context.Context, vscName string, + backupName string, ) bool { existing := new(snapshotv1api.VolumeSnapshotContent) if err := p.crClient.Get(ctx, crclient.ObjectKey{Name: vscName}, existing); err != nil { @@ -164,6 +165,15 @@ func (p *volumeSnapshotContentDeleteItemAction) tryDeleteOriginalVSC( return false } + if !kubeutil.HasBackupLabel(&existing.ObjectMeta, backupName) { + p.log.Warnf( + "Original VolumeSnapshotContent %s in cluster does not belong to backup %s, skipping direct deletion", + vscName, + backupName, + ) + return false + } + p.log.Debugf("Found original VolumeSnapshotContent %s in cluster (legacy backup), cleaning up directly", vscName) // Patch DeletionPolicy to Delete so the CSI driver removes the cloud snapshot diff --git a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go index e8a0b5865..25cc69b82 100644 --- a/internal/delete/actions/csi/volumesnapshotcontent_action_test.go +++ b/internal/delete/actions/csi/volumesnapshotcontent_action_test.go @@ -122,7 +122,29 @@ func TestVSCExecute(t *testing.T) { backup: builder.ForBackup("velero", "backup").Result(), expectErr: false, preExistingVSC: &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{Name: "bar"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Labels: map[string]string{ + velerov1api.BackupNameLabel: "backup", + }, + }, + Spec: snapshotv1api.VolumeSnapshotContentSpec{ + DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, + Driver: "disk.csi.azure.com", + Source: snapshotv1api.VolumeSnapshotContentSource{SnapshotHandle: stringPtr("snap-123")}, + VolumeSnapshotRef: corev1api.ObjectReference{Name: "vs-1", Namespace: "default"}, + }, + }, + }, + { + name: "Original VSC exists in cluster without backup label, falls through to temp VSC flow", + vsc: builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(), + backup: builder.ForBackup("velero", "backup").Result(), + expectErr: false, + preExistingVSC: &snapshotv1api.VolumeSnapshotContent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + }, Spec: snapshotv1api.VolumeSnapshotContentSpec{ DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, Driver: "disk.csi.azure.com", @@ -200,22 +222,51 @@ func TestNewVolumeSnapshotContentDeleteItemAction(t *testing.T) { func TestTryDeleteOriginalVSC(t *testing.T) { tests := []struct { - name string - vscName string - existing *snapshotv1api.VolumeSnapshotContent - createIt bool - expectRet bool + name string + vscName string + backupName string + existing *snapshotv1api.VolumeSnapshotContent + createIt bool + expectRet bool }{ { - name: "VSC not found in cluster, returns false", - vscName: "not-found", + name: "VSC not found in cluster, returns false", + vscName: "not-found", + backupName: "test-backup", + expectRet: false, + }, + { + name: "VSC found in cluster without backup label, returns false", + vscName: "unlabeled-vsc", + backupName: "test-backup", + existing: &snapshotv1api.VolumeSnapshotContent{ + ObjectMeta: metav1.ObjectMeta{Name: "unlabeled-vsc"}, + Spec: snapshotv1api.VolumeSnapshotContentSpec{ + DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, + Driver: "disk.csi.azure.com", + Source: snapshotv1api.VolumeSnapshotContentSource{ + SnapshotHandle: stringPtr("snap-123"), + }, + VolumeSnapshotRef: corev1api.ObjectReference{ + Name: "vs-1", + Namespace: "default", + }, + }, + }, + createIt: true, expectRet: false, }, { - name: "VSC found with Retain policy, patches and deletes", - vscName: "legacy-vsc", + name: "VSC found with Retain policy and matching backup label, patches and deletes", + vscName: "legacy-vsc", + backupName: "test-backup", existing: &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{Name: "legacy-vsc"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "legacy-vsc", + Labels: map[string]string{ + velerov1api.BackupNameLabel: "test-backup", + }, + }, Spec: snapshotv1api.VolumeSnapshotContentSpec{ DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, Driver: "disk.csi.azure.com", @@ -232,10 +283,16 @@ func TestTryDeleteOriginalVSC(t *testing.T) { expectRet: true, }, { - name: "VSC found with Delete policy already, just deletes", - vscName: "already-delete-vsc", + name: "VSC found with Delete policy and matching backup label, just deletes", + vscName: "already-delete-vsc", + backupName: "test-backup", existing: &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{Name: "already-delete-vsc"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "already-delete-vsc", + Labels: map[string]string{ + velerov1api.BackupNameLabel: "test-backup", + }, + }, Spec: snapshotv1api.VolumeSnapshotContentSpec{ DeletionPolicy: snapshotv1api.VolumeSnapshotContentDelete, Driver: "disk.csi.azure.com", @@ -266,7 +323,7 @@ func TestTryDeleteOriginalVSC(t *testing.T) { require.NoError(t, crClient.Create(t.Context(), test.existing)) } - result := p.tryDeleteOriginalVSC(t.Context(), test.vscName) + result := p.tryDeleteOriginalVSC(t.Context(), test.vscName, test.backupName) require.Equal(t, test.expectRet, result) // If cleanup succeeded, verify the VSC is gone @@ -289,13 +346,18 @@ func TestTryDeleteOriginalVSC(t *testing.T) { log: logrus.StandardLogger(), crClient: errClient, } - require.False(t, p.tryDeleteOriginalVSC(t.Context(), "some-vsc")) + require.False(t, p.tryDeleteOriginalVSC(t.Context(), "some-vsc", "test-backup")) }) t.Run("Patch fails, returns false", func(t *testing.T) { realClient := velerotest.NewFakeControllerRuntimeClient(t) vsc := &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{Name: "patch-fail-vsc"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "patch-fail-vsc", + Labels: map[string]string{ + velerov1api.BackupNameLabel: "test-backup", + }, + }, Spec: snapshotv1api.VolumeSnapshotContentSpec{ DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, Driver: "disk.csi.azure.com", @@ -313,13 +375,18 @@ func TestTryDeleteOriginalVSC(t *testing.T) { log: logrus.StandardLogger(), crClient: errClient, } - require.False(t, p.tryDeleteOriginalVSC(t.Context(), "patch-fail-vsc")) + require.False(t, p.tryDeleteOriginalVSC(t.Context(), "patch-fail-vsc", "test-backup")) }) t.Run("Delete fails, returns false", func(t *testing.T) { realClient := velerotest.NewFakeControllerRuntimeClient(t) vsc := &snapshotv1api.VolumeSnapshotContent{ - ObjectMeta: metav1.ObjectMeta{Name: "delete-fail-vsc"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "delete-fail-vsc", + Labels: map[string]string{ + velerov1api.BackupNameLabel: "test-backup", + }, + }, Spec: snapshotv1api.VolumeSnapshotContentSpec{ DeletionPolicy: snapshotv1api.VolumeSnapshotContentDelete, Driver: "disk.csi.azure.com", @@ -337,7 +404,7 @@ func TestTryDeleteOriginalVSC(t *testing.T) { log: logrus.StandardLogger(), crClient: errClient, } - require.False(t, p.tryDeleteOriginalVSC(t.Context(), "delete-fail-vsc")) + require.False(t, p.tryDeleteOriginalVSC(t.Context(), "delete-fail-vsc", "test-backup")) }) }