From 0291c53e9d95530c2e3bfdb628f72496356269b2 Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Wed, 8 Apr 2026 15:02:27 +0800 Subject: [PATCH] Fix PodVolumeBackup list scope during restore Restrict the listing of PodVolumeBackup resources to the specific restore namespace in both the core restore controller and the pod volume restore action plugin. This prevents "Forbidden" errors when Velero is configured with namespace-scoped minimum privileges, avoiding the need for cluster-scoped list permissions for PodVolumeBackups. Fixes: #9681 Signed-off-by: Adam Zhang --- changelogs/unreleased/9696-adam-jian-zhang | 1 + pkg/controller/restore_controller.go | 1 + pkg/controller/restore_controller_test.go | 35 +++++++++++++++++-- .../actions/pod_volume_restore_action.go | 1 + .../actions/pod_volume_restore_action_test.go | 22 ++++++++++++ 5 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/9696-adam-jian-zhang diff --git a/changelogs/unreleased/9696-adam-jian-zhang b/changelogs/unreleased/9696-adam-jian-zhang new file mode 100644 index 000000000..0cb724f5b --- /dev/null +++ b/changelogs/unreleased/9696-adam-jian-zhang @@ -0,0 +1 @@ +Fix issue #9681, fix restores and podvolumerestores list options to only list in installed namespace diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 208a3ddca..469951f27 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -529,6 +529,7 @@ func (r *restoreReconciler) runValidatedRestore(restore *api.Restore, info backu LabelSelector: labels.Set(map[string]string{ api.BackupNameLabel: label.GetValidName(restore.Spec.BackupName), }).AsSelector(), + Namespace: restore.Namespace, } podVolumeBackupList := &api.PodVolumeBackupList{} diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 3acc03d2a..b013ee64d 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -238,6 +238,8 @@ func TestRestoreReconcile(t *testing.T) { expectedFinalPhase string addValidFinalizer bool emptyVolumeInfo bool + podVolumeBackups []*velerov1api.PodVolumeBackup + expectedPVBCount int }{ { name: "restore with both namespace in both includedNamespaces and excludedNamespaces fails validation", @@ -357,6 +359,22 @@ func TestRestoreReconcile(t *testing.T) { expectedCompletedTime: ×tamp, expectedRestorerCall: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).Result(), }, + { + name: "valid restore gets executed and only includes pod volume backups from restore namespace", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar2", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), + backup: defaultBackup().StorageLocation("default").Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("foo", "pvb-1").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1")).Result(), + builder.ForPodVolumeBackup("other-ns", "pvb-2").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1")).Result(), + }, + expectedPVBCount: 1, + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseInProgress), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: NewRestore("foo", "bar2", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).Result(), + }, { name: "restoration of nodes is not supported", location: defaultStorageLocation, @@ -501,6 +519,13 @@ func TestRestoreReconcile(t *testing.T) { defaultStorageLocation.ObjectMeta.ResourceVersion = "" }() + if test.podVolumeBackups != nil { + for _, pvb := range test.podVolumeBackups { + err := fakeClient.Create(t.Context(), pvb) + require.NoError(t, err) + } + } + r := NewRestoreReconciler( t.Context(), velerov1api.DefaultNamespace, @@ -670,6 +695,10 @@ func TestRestoreReconcile(t *testing.T) { // the mock stores the pointer, which gets modified after assert.Equal(t, test.expectedRestorerCall.Spec, restorer.calledWithArg.Spec) assert.Equal(t, test.expectedRestorerCall.Status.Phase, restorer.calledWithArg.Status.Phase) + + if test.podVolumeBackups != nil { + assert.Len(t, restorer.calledWithPVBs, test.expectedPVBCount) + } }) } } @@ -1021,8 +1050,9 @@ func NewRestore(ns, name, backup, includeNS, includeResource string, phase veler type fakeRestorer struct { mock.Mock - calledWithArg velerov1api.Restore - kbClient client.Client + calledWithArg velerov1api.Restore + calledWithPVBs []*velerov1api.PodVolumeBackup + kbClient client.Client } func (r *fakeRestorer) Restore( @@ -1045,6 +1075,7 @@ func (r *fakeRestorer) RestoreWithResolvers(req *pkgrestore.Request, r.kbClient, volumeSnapshotterGetter) r.calledWithArg = *req.Restore + r.calledWithPVBs = req.PodVolumeBackups return res.Get(0).(results.Result), res.Get(1).(results.Result) } diff --git a/pkg/restore/actions/pod_volume_restore_action.go b/pkg/restore/actions/pod_volume_restore_action.go index 4e3180fef..e26a53034 100644 --- a/pkg/restore/actions/pod_volume_restore_action.go +++ b/pkg/restore/actions/pod_volume_restore_action.go @@ -101,6 +101,7 @@ func (a *PodVolumeRestoreAction) Execute(input *velero.RestoreItemActionExecuteI opts := &ctrlclient.ListOptions{ LabelSelector: label.NewSelectorForBackup(input.Restore.Spec.BackupName), + Namespace: input.Restore.Namespace, } podVolumeBackupList := new(velerov1api.PodVolumeBackupList) if err := a.crClient.List(context.TODO(), podVolumeBackupList, opts); err != nil { diff --git a/pkg/restore/actions/pod_volume_restore_action_test.go b/pkg/restore/actions/pod_volume_restore_action_test.go index 70911ca37..bc9662ab7 100644 --- a/pkg/restore/actions/pod_volume_restore_action_test.go +++ b/pkg/restore/actions/pod_volume_restore_action_test.go @@ -350,6 +350,28 @@ func TestPodVolumeRestoreActionExecute(t *testing.T) { VolumeMounts(builder.ForVolumeMount("myvol", "/restores/myvol").Result()). Command([]string{"/velero-restore-helper"}).Result()).Result(), }, + { + name: "pod volume backups in a different namespace are ignored when looking for matches due to namespace scoping", + pod: builder.ForPod("ns-1", "my-pod"). + Volumes( + builder.ForVolume("myvol").PersistentVolumeClaimSource("pvc-1").Result(), + ). + Result(), + podVolumeBackups: []runtime.Object{ + builder.ForPodVolumeBackup("other-ns", "pvb-1"). + PodName("my-pod"). + PodNamespace("ns-1"). + Volume("myvol"). + ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, backupName)). + SnapshotID("foo"). + Result(), + }, + want: builder.ForPod("ns-1", "my-pod"). + Volumes( + builder.ForVolume("myvol").PersistentVolumeClaimSource("pvc-1").Result(), + ). + Result(), + }, } veleroDeployment := &appsv1api.Deployment{