diff --git a/changelogs/unreleased/3475-zubron b/changelogs/unreleased/3475-zubron new file mode 100644 index 000000000..ed00ab9ca --- /dev/null +++ b/changelogs/unreleased/3475-zubron @@ -0,0 +1 @@ +Fixed a bug where restic volumes would not be restored when using a namespace mapping. \ No newline at end of file diff --git a/pkg/restic/common.go b/pkg/restic/common.go index 592f3a3b7..d248fe922 100644 --- a/pkg/restic/common.go +++ b/pkg/restic/common.go @@ -96,17 +96,17 @@ func getPodSnapshotAnnotations(obj metav1.Object) map[string]string { return res } -func isPVBMatchPod(pvb *velerov1api.PodVolumeBackup, pod metav1.Object) bool { - return pod.GetName() == pvb.Spec.Pod.Name && pod.GetNamespace() == pvb.Spec.Pod.Namespace +func isPVBMatchPod(pvb *velerov1api.PodVolumeBackup, podName string, namespace string) bool { + return podName == pvb.Spec.Pod.Name && namespace == pvb.Spec.Pod.Namespace } // GetVolumeBackupsForPod returns a map, of volume name -> snapshot id, // of the PodVolumeBackups that exist for the provided pod. -func GetVolumeBackupsForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, pod metav1.Object) map[string]string { +func GetVolumeBackupsForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, pod metav1.Object, sourcePodNs string) map[string]string { volumes := make(map[string]string) for _, pvb := range podVolumeBackups { - if !isPVBMatchPod(pvb, pod) { + if !isPVBMatchPod(pvb, pod.GetName(), sourcePodNs) { continue } diff --git a/pkg/restic/common_test.go b/pkg/restic/common_test.go index cde5e9d37..25e4d8ba4 100644 --- a/pkg/restic/common_test.go +++ b/pkg/restic/common_test.go @@ -40,78 +40,93 @@ func TestGetVolumeBackupsForPod(t *testing.T) { podVolumeBackups []*velerov1api.PodVolumeBackup podAnnotations map[string]string podName string + sourcePodNs string expected map[string]string }{ { - name: "nil annotations", + name: "nil annotations results in no volume backups returned", podAnnotations: nil, expected: nil, }, { - name: "empty annotations", + name: "empty annotations results in no volume backups returned", podAnnotations: make(map[string]string), expected: nil, }, { - name: "non-empty map, no snapshot annotation", + name: "pod annotations with no snapshot annotation prefix results in no volume backups returned", podAnnotations: map[string]string{"foo": "bar"}, expected: nil, }, { - name: "has snapshot annotation only, no suffix", - podAnnotations: map[string]string{podAnnotationPrefix: "bar"}, - expected: map[string]string{"": "bar"}, + name: "pod annotation with only snapshot annotation prefix, results in volume backup with empty volume key", + podAnnotations: map[string]string{podAnnotationPrefix: "snapshotID"}, + expected: map[string]string{"": "snapshotID"}, }, { - name: "has snapshot annotation only, with suffix", - podAnnotations: map[string]string{podAnnotationPrefix + "foo": "bar"}, - expected: map[string]string{"foo": "bar"}, + name: "pod annotation with snapshot annotation prefix results in volume backup with volume name and snapshot ID", + podAnnotations: map[string]string{podAnnotationPrefix + "volume": "snapshotID"}, + expected: map[string]string{"volume": "snapshotID"}, }, { - name: "has snapshot annotation, with suffix", - podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "foo": "bar", podAnnotationPrefix + "abc": "123"}, - expected: map[string]string{"foo": "bar", "abc": "123"}, + name: "only pod annotations with snapshot annotation prefix are considered", + podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "volume1": "snapshot1", podAnnotationPrefix + "volume2": "snapshot2"}, + expected: map[string]string{"volume1": "snapshot1", "volume2": "snapshot2"}, }, { - name: "has snapshot annotation, with suffix, and also PVBs", + name: "pod annotations are not considered if PVBs are provided", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), }, podName: "TestPod", + sourcePodNs: "TestNS", podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "foo": "bar", podAnnotationPrefix + "abc": "123"}, - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, }, { - name: "no snapshot annotation, but with PVBs", + name: "volume backups are returned even if no pod annotations are present", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), }, - podName: "TestPod", - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, }, { - name: "no snapshot annotation, but with PVBs, some of which have snapshot IDs and some of which don't", + name: "only volumes from PVBs with snapshot IDs are returned", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), - builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestPod").Volume("pvbtest3-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-4").PodName("TestPod").Volume("pvbtest4-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestPod").PodNamespace("TestNS").Volume("pvbtest3-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-4").PodName("TestPod").PodNamespace("TestNS").Volume("pvbtest4-abc").Result(), }, - podName: "TestPod", - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, }, { - name: "has snapshot annotation, with suffix, and with PVBs from current pod and a PVB from another pod", + name: "only volumes from PVBs for the given pod are returned", podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").SnapshotID("bar").Volume("pvbtest1-foo").Result(), - builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").SnapshotID("123").Volume("pvbtest2-abc").Result(), - builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestAnotherPod").SnapshotID("xyz").Volume("pvbtest3-xyz").Result(), + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestAnotherPod").SnapshotID("snapshot3").Volume("pvbtest3-xyz").Result(), }, - podAnnotations: map[string]string{"x": "y", podAnnotationPrefix + "foo": "bar", podAnnotationPrefix + "abc": "123"}, - podName: "TestPod", - expected: map[string]string{"pvbtest1-foo": "bar", "pvbtest2-abc": "123"}, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1", "pvbtest2-abc": "snapshot2"}, + }, + { + name: "only volumes from PVBs which match the pod name and source pod namespace are returned", + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("velero", "pvb-1").PodName("TestPod").PodNamespace("TestNS").SnapshotID("snapshot1").Volume("pvbtest1-foo").Result(), + builder.ForPodVolumeBackup("velero", "pvb-2").PodName("TestAnotherPod").PodNamespace("TestNS").SnapshotID("snapshot2").Volume("pvbtest2-abc").Result(), + builder.ForPodVolumeBackup("velero", "pvb-3").PodName("TestPod").PodNamespace("TestAnotherNS").SnapshotID("snapshot3").Volume("pvbtest3-xyz").Result(), + }, + podName: "TestPod", + sourcePodNs: "TestNS", + expected: map[string]string{"pvbtest1-foo": "snapshot1"}, }, } @@ -121,7 +136,7 @@ func TestGetVolumeBackupsForPod(t *testing.T) { pod.Annotations = test.podAnnotations pod.Name = test.podName - res := GetVolumeBackupsForPod(test.podVolumeBackups, pod) + res := GetVolumeBackupsForPod(test.podVolumeBackups, pod, test.sourcePodNs) assert.Equal(t, test.expected, res) }) } @@ -558,17 +573,14 @@ func TestGetPodVolumesUsingRestic(t *testing.T) { func TestIsPVBMatchPod(t *testing.T) { testCases := []struct { - name string - pod metav1.Object - pvb velerov1api.PodVolumeBackup - expected bool + name string + pvb velerov1api.PodVolumeBackup + podName string + sourcePodNs string + expected bool }{ { name: "should match PVB and pod", - pod: &metav1.ObjectMeta{ - Name: "matching-pod", - Namespace: "matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -577,14 +589,12 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: true, + podName: "matching-pod", + sourcePodNs: "matching-namespace", + expected: true, }, { name: "should not match PVB and pod, pod name mismatch", - pod: &metav1.ObjectMeta{ - Name: "not-matching-pod", - Namespace: "matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -593,14 +603,12 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: false, + podName: "not-matching-pod", + sourcePodNs: "matching-namespace", + expected: false, }, { name: "should not match PVB and pod, pod namespace mismatch", - pod: &metav1.ObjectMeta{ - Name: "matching-pod", - Namespace: "not-matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -609,14 +617,12 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: false, + podName: "matching-pod", + sourcePodNs: "not-matching-namespace", + expected: false, }, { name: "should not match PVB and pod, pod name and namespace mismatch", - pod: &metav1.ObjectMeta{ - Name: "not-matching-pod", - Namespace: "not-matching-namespace", - }, pvb: velerov1api.PodVolumeBackup{ Spec: velerov1api.PodVolumeBackupSpec{ Pod: corev1api.ObjectReference{ @@ -625,13 +631,15 @@ func TestIsPVBMatchPod(t *testing.T) { }, }, }, - expected: false, + podName: "not-matching-pod", + sourcePodNs: "not-matching-namespace", + expected: false, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - actual := isPVBMatchPod(&tc.pvb, tc.pod) + actual := isPVBMatchPod(&tc.pvb, tc.podName, tc.sourcePodNs) assert.Equal(t, tc.expected, actual) }) diff --git a/pkg/restic/restorer.go b/pkg/restic/restorer.go index a17cfeea2..049d734e7 100644 --- a/pkg/restic/restorer.go +++ b/pkg/restic/restorer.go @@ -92,7 +92,7 @@ func newRestorer( } func (r *restorer) RestorePodVolumes(data RestoreData) []error { - volumesToRestore := GetVolumeBackupsForPod(data.PodVolumeBackups, data.Pod) + volumesToRestore := GetVolumeBackupsForPod(data.PodVolumeBackups, data.Pod, data.SourceNamespace) if len(volumesToRestore) == 0 { return nil } diff --git a/pkg/restore/restic_restore_action.go b/pkg/restore/restic_restore_action.go index 38aebd72c..42c755a93 100644 --- a/pkg/restore/restic_restore_action.go +++ b/pkg/restore/restic_restore_action.go @@ -76,6 +76,15 @@ func (a *ResticRestoreAction) Execute(input *velero.RestoreItemActionExecuteInpu return nil, errors.Wrap(err, "unable to convert pod from runtime.Unstructured") } + // At the point when this function is called, the namespace mapping for the restore + // has not yet been applied to `input.Item` so we can't perform a reverse-lookup in + // the namespace mapping in the restore spec. Instead, use the pod from the backup + // so that if the mapping is applied earlier, we still use the correct namespace. + var podFromBackup corev1.Pod + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(input.ItemFromBackup.UnstructuredContent(), &podFromBackup); err != nil { + return nil, errors.Wrap(err, "unable to convert source pod from runtime.Unstructured") + } + log := a.logger.WithField("pod", kube.NamespaceAndName(&pod)) opts := label.NewListOptionsForBackup(input.Restore.Spec.BackupName) @@ -88,7 +97,7 @@ func (a *ResticRestoreAction) Execute(input *velero.RestoreItemActionExecuteInpu for i := range podVolumeBackupList.Items { podVolumeBackups = append(podVolumeBackups, &podVolumeBackupList.Items[i]) } - volumeSnapshots := restic.GetVolumeBackupsForPod(podVolumeBackups, &pod) + volumeSnapshots := restic.GetVolumeBackupsForPod(podVolumeBackups, &pod, podFromBackup.Namespace) if len(volumeSnapshots) == 0 { log.Debug("No restic backups found for pod") return velero.NewRestoreItemActionExecuteOutput(input.Item), nil diff --git a/pkg/restore/restic_restore_action_test.go b/pkg/restore/restic_restore_action_test.go index 0211cdcba..dc1c65acc 100644 --- a/pkg/restore/restic_restore_action_test.go +++ b/pkg/restore/restic_restore_action_test.go @@ -122,6 +122,7 @@ func TestResticRestoreActionExecute(t *testing.T) { tests := []struct { name string pod *corev1api.Pod + podFromBackup *corev1api.Pod podVolumeBackups []*velerov1api.PodVolumeBackup want *corev1api.Pod }{ @@ -202,6 +203,49 @@ func TestResticRestoreActionExecute(t *testing.T) { builder.ForContainer("first-container", "").Result()). Result(), }, + { + name: "Restoring pod in another namespace adds the restic initContainer and uses the namespace of the backup pod for matching PVBs", + pod: builder.ForPod("new-ns", "my-pod"). + Volumes( + builder.ForVolume("vol-1").PersistentVolumeClaimSource("pvc-1").Result(), + builder.ForVolume("vol-2").PersistentVolumeClaimSource("pvc-2").Result(), + ). + Result(), + podFromBackup: builder.ForPod("original-ns", "my-pod"). + Volumes( + builder.ForVolume("vol-1").PersistentVolumeClaimSource("pvc-1").Result(), + builder.ForVolume("vol-2").PersistentVolumeClaimSource("pvc-2").Result(), + ). + Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup(veleroNs, "pvb-1"). + PodName("my-pod"). + PodNamespace("original-ns"). + Volume("vol-1"). + ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, backupName)). + SnapshotID("foo"). + Result(), + builder.ForPodVolumeBackup(veleroNs, "pvb-2"). + PodName("my-pod"). + PodNamespace("original-ns"). + Volume("vol-2"). + ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, backupName)). + SnapshotID("foo"). + Result(), + }, + want: builder.ForPod("new-ns", "my-pod"). + Volumes( + builder.ForVolume("vol-1").PersistentVolumeClaimSource("pvc-1").Result(), + builder.ForVolume("vol-2").PersistentVolumeClaimSource("pvc-2").Result(), + ). + InitContainers( + newResticInitContainerBuilder(initContainerImage(defaultImageBase), ""). + Resources(&resourceReqs). + SecurityContext(&securityContext). + VolumeMounts(builder.ForVolumeMount("vol-1", "/restores/vol-1").Result(), builder.ForVolumeMount("vol-2", "/restores/vol-2").Result()). + Command([]string{"/velero-restic-restore-helper"}).Result()). + Result(), + }, } for _, tc := range tests { @@ -214,12 +258,24 @@ func TestResticRestoreActionExecute(t *testing.T) { require.NoError(t, err) } - unstructuredMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pod) + unstructuredPod, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pod) require.NoError(t, err) + // Default to using the same pod for both Item and ItemFromBackup if podFromBackup not provided + var unstructuredPodFromBackup map[string]interface{} + if tc.podFromBackup != nil { + unstructuredPodFromBackup, err = runtime.DefaultUnstructuredConverter.ToUnstructured(tc.podFromBackup) + require.NoError(t, err) + } else { + unstructuredPodFromBackup = unstructuredPod + } + input := &velero.RestoreItemActionExecuteInput{ Item: &unstructured.Unstructured{ - Object: unstructuredMap, + Object: unstructuredPod, + }, + ItemFromBackup: &unstructured.Unstructured{ + Object: unstructuredPodFromBackup, }, Restore: builder.ForRestore(veleroNs, restoreName). Backup(backupName). diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 89605bd51..c6ab9dd9e 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1197,7 +1197,7 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs } - if groupResource == kuberesource.Pods && len(restic.GetVolumeBackupsForPod(ctx.podVolumeBackups, obj)) > 0 { + if groupResource == kuberesource.Pods && len(restic.GetVolumeBackupsForPod(ctx.podVolumeBackups, obj, originalNamespace)) > 0 { restorePodVolumeBackups(ctx, createdObj, originalNamespace) }