diff --git a/changelogs/unreleased/9903-Joeavaikath b/changelogs/unreleased/9903-Joeavaikath new file mode 100644 index 000000000..d586acfaa --- /dev/null +++ b/changelogs/unreleased/9903-Joeavaikath @@ -0,0 +1 @@ +Fix restore finalization overwriting dynamically provisioned PV labels with stale backup values diff --git a/pkg/controller/restore_finalizer_controller.go b/pkg/controller/restore_finalizer_controller.go index 20e4bc849..4e02bb0ef 100644 --- a/pkg/controller/restore_finalizer_controller.go +++ b/pkg/controller/restore_finalizer_controller.go @@ -421,7 +421,16 @@ func (ctx *finalizerContext) patchDynamicPVWithVolumeInfo() (errs results.Result // patch PV's reclaim policy and label using the corresponding data stored in volume info if needPatch(pv, volInfo.PVInfo) { updatedPV := pv.DeepCopy() - updatedPV.Labels = volInfo.PVInfo.Labels + + if updatedPV.Labels == nil { + updatedPV.Labels = make(map[string]string) + } + for k, v := range volInfo.PVInfo.Labels { + if _, exists := updatedPV.Labels[k]; !exists { + updatedPV.Labels[k] = v + } + } + updatedPV.Spec.PersistentVolumeReclaimPolicy = corev1api.PersistentVolumeReclaimPolicy(volInfo.PVInfo.ReclaimPolicy) if err := kubeutil.PatchResource(pv, updatedPV, ctx.crClient); err != nil { return false, err @@ -553,13 +562,10 @@ func needPatch(newPV *corev1api.PersistentVolume, pvInfo *volume.PVInfo) bool { } newPVLabels, pvLabels := newPV.Labels, pvInfo.Labels - for k, v := range pvLabels { + for k := range pvLabels { if _, ok := newPVLabels[k]; !ok { return true } - if newPVLabels[k] != v { - return true - } } return false diff --git a/pkg/controller/restore_finalizer_controller_test.go b/pkg/controller/restore_finalizer_controller_test.go index f07d2576c..8f2618f9d 100644 --- a/pkg/controller/restore_finalizer_controller_test.go +++ b/pkg/controller/restore_finalizer_controller_test.go @@ -634,6 +634,87 @@ func Test_restoreFinalizerReconciler_finishProcessing(t *testing.T) { } } +func TestNeedPatch(t *testing.T) { + tests := []struct { + name string + newPV *corev1api.PersistentVolume + pvInfo *volume.PVInfo + expected bool + }{ + { + name: "reclaim policy differs", + newPV: builder.ForPersistentVolume("pv1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimRetain), + Labels: map[string]string{}, + }, + expected: true, + }, + { + name: "backup has label new PV does not", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("existing", "val")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{"existing": "val", "missing": "val"}, + }, + expected: true, + }, + { + name: "same labels same values", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("key", "val")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{"key": "val"}, + }, + expected: false, + }, + { + name: "same label key different values", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("topology.kubernetes.io/zone", "us-west-2a")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{"topology.kubernetes.io/zone": "us-east-1a"}, + }, + expected: false, + }, + { + name: "new PV has labels backup does not", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("provisioner-label", "val")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{}, + }, + expected: false, + }, + { + name: "both labels nil", + newPV: builder.ForPersistentVolume("pv1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: nil, + }, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := needPatch(tc.newPV, tc.pvInfo) + assert.Equal(t, tc.expected, result) + }) + } +} + func TestRestoreOperationList(t *testing.T) { var empty []*itemoperation.RestoreOperation tests := []struct {