Use pod namespace from backup when matching PVBs (#3475)

* Use pod namespace from backup when matching PVBs

In #3051, we introduced an additional check to ensure that a PVB matched
a particular pod by checking both the name and the namespace of the pod.
This caused an issue when using a namespace mapping on restore. In the
case where a namespace mapping is being used, the check for whether a
PVB matches a particular pod will fail as the PVB was created for the
original pod namespace and is not aware of the new namespace mapping
being used. This resulted in PVRs not being created for pods that were
being restored into new namespaces. The restic init containers were
being created to wait on the volume restore, however this would cause
the restored pods to block indefinitely as they would be waiting for a
volume restore that was not scheduled.

To fix this, we use the original namespace of the pod from the backup to
match the PVB to the pod being restored, not the new namespace where
the pod is being restored into.

Fixes #3467.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

* Explain why the namespace mapping can't be used

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>

changelogs/unreleased/3475-zubron

@@ -0,0 +1 @@
+Fixed a bug where restic volumes would not be restored when using a namespace mapping.

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
 		}
 

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)
 		})
 

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
 	}

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

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).

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)
 	}