From c9c7541c82118fbac28b12dae5c95a1ae326905d Mon Sep 17 00:00:00 2001 From: Baudouin Herlicq Date: Fri, 16 Aug 2024 14:10:57 +0200 Subject: [PATCH 1/4] Update restore logic to allow Resource Modifiers to use data from the status field Signed-off-by: Baudouin Herlicq --- changelogs/unreleased/8116-BaudouinH | 1 + pkg/restore/restore.go | 18 +-- pkg/restore/restore_test.go | 170 +++++++++++++++++++++++++++ 3 files changed, 180 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/8116-BaudouinH diff --git a/changelogs/unreleased/8116-BaudouinH b/changelogs/unreleased/8116-BaudouinH new file mode 100644 index 0000000000..f9107bdd8a --- /dev/null +++ b/changelogs/unreleased/8116-BaudouinH @@ -0,0 +1 @@ +Change restore flow to allow resourceModifiers to use status field in their patches. \ No newline at end of file diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 63c26538ba..4a3bd6abc4 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1294,15 +1294,6 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } } - objStatus, statusFieldExists, statusFieldErr := unstructured.NestedFieldCopy(obj.Object, "status") - // Clear out non-core metadata fields and status. - if obj, err = resetMetadataAndStatus(obj); err != nil { - errs.Add(namespace, err) - return warnings, errs, itemExists - } - - ctx.log.Infof("restore status includes excludes: %+v", ctx.resourceStatusIncludesExcludes) - for _, action := range ctx.getApplicableActions(groupResource, namespace) { if !action.Selector.Matches(labels.Set(obj.GetLabels())) { continue @@ -1451,6 +1442,15 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } } + // Status is removed after the ResourceModifiers are applied as some resource modifiers may rely on status fields to modify the object. + objStatus, statusFieldExists, statusFieldErr := unstructured.NestedFieldCopy(obj.Object, "status") + // Clear out non-core metadata fields and status. + if obj, err = resetMetadataAndStatus(obj); err != nil { + errs.Add(namespace, err) + return warnings, errs, itemExists + } + ctx.log.Infof("restore status includes excludes: %+v", ctx.resourceStatusIncludesExcludes) + // Necessary because we may have remapped the namespace if the namespace is // blank, don't create the key. originalNamespace := obj.GetNamespace() diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 4aa0d6d6a8..0c07fca3ef 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -42,6 +42,7 @@ import ( "k8s.io/client-go/dynamic" kubetesting "k8s.io/client-go/testing" + "github.com/vmware-tanzu/velero/internal/resourcemodifiers" "github.com/vmware-tanzu/velero/internal/volume" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/archive" @@ -4109,3 +4110,172 @@ func TestHasSnapshotDataUpload(t *testing.T) { }) } } + +func TestRestoreResourceModifiers(t *testing.T) { + tests := []struct { + name string + restore *velerov1api.Restore + backup *velerov1api.Backup + apiResources []*test.APIResource + tarball io.Reader + want []*test.APIResource + expectedRestoreItems map[itemKey]restoredItemStatus + resourceModifiers *resourcemodifiers.ResourceModifiers + }{ + { + name: "modify image", + restore: defaultRestore().Result(), + backup: defaultBackup().Result(), + resourceModifiers: &resourcemodifiers.ResourceModifiers{ + ResourceModifierRules: []resourcemodifiers.ResourceModifierRule{ + { + Conditions: resourcemodifiers.Conditions{ + GroupResource: "pods", + }, + StrategicPatches: []resourcemodifiers.StrategicMergePatch{ + { + PatchData: `{"spec":{"containers":[{"name":"test-container","image":"nginx1"}]}}`, + }, + }, + }, + }, + }, + tarball: test.NewTarWriter(t). + AddItems("pods", + builder.ForPod("ns-1", "pod-1"). + Containers(&corev1api.Container{Name: "test-container", Image: "nginx"}). + Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.Pods(), + }, + want: []*test.APIResource{ + test.Pods( + builder.ForPod("ns-1", "pod-1"). + Containers(&corev1api.Container{Name: "test-container", Image: "nginx1"}). + ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1", velerov1api.RestoreNameLabel, "restore-1")). + Result(), + ), + }, + expectedRestoreItems: map[itemKey]restoredItemStatus{ + {resource: "v1/Namespace", namespace: "", name: "ns-1"}: {action: "created", itemExists: true}, + {resource: "v1/Pod", namespace: "ns-1", name: "pod-1"}: {action: "created", itemExists: true}, + }, + }, + { + name: "update label using another label", + restore: defaultRestore().Result(), + backup: defaultBackup().Result(), + resourceModifiers: &resourcemodifiers.ResourceModifiers{ + ResourceModifierRules: []resourcemodifiers.ResourceModifierRule{ + { + Conditions: resourcemodifiers.Conditions{ + GroupResource: "pods", + }, + Patches: []resourcemodifiers.JSONPatch{ + { + Operation: "copy", + Path: "/metadata/labels/test-annotation-2", + From: "/metadata/labels/test-annotation-1", + }, + }, + }, + }, + }, + tarball: test.NewTarWriter(t). + AddItems("pods", + builder.ForPod("ns-1", "pod-1"). + ObjectMeta(builder.WithLabels("test-annotation-1", "something")). + Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.Pods(), + }, + want: []*test.APIResource{ + test.Pods( + builder.ForPod("ns-1", "pod-1").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1", velerov1api.RestoreNameLabel, "restore-1", "test-annotation-1", "something", "test-annotation-2", "something")).Result(), + ), + }, + expectedRestoreItems: map[itemKey]restoredItemStatus{ + {resource: "v1/Namespace", namespace: "", name: "ns-1"}: {action: "created", itemExists: true}, + {resource: "v1/Pod", namespace: "ns-1", name: "pod-1"}: {action: "created", itemExists: true}, + }, + }, + { + name: "update label using status field", + restore: defaultRestore().Result(), + backup: defaultBackup().Result(), + resourceModifiers: &resourcemodifiers.ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []resourcemodifiers.ResourceModifierRule{ + { + Conditions: resourcemodifiers.Conditions{ + GroupResource: "pods", + }, + Patches: []resourcemodifiers.JSONPatch{ + { + Operation: "copy", + Path: "/metadata/labels/test-annotation-1", + From: "/status/message", + }, + }, + }, + }, + }, + tarball: test.NewTarWriter(t). + AddItems("pods", + builder.ForPod("ns-1", "pod-1"). + ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1", velerov1api.RestoreNameLabel, "restore-1", "test-annotation-1", "another thing")). + Status(corev1api.PodStatus{Message: "something"}). + Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.Pods(), + }, + want: []*test.APIResource{ + test.Pods( + builder.ForPod("ns-1", "pod-1").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1", velerov1api.RestoreNameLabel, "restore-1", "test-annotation-1", "something")).Result(), + ), + }, + expectedRestoreItems: map[itemKey]restoredItemStatus{ + {resource: "v1/Namespace", namespace: "", name: "ns-1"}: {action: "created", itemExists: true}, + {resource: "v1/Pod", namespace: "ns-1", name: "pod-1"}: {action: "created", itemExists: true}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h := newHarness(t) + + for _, r := range tc.apiResources { + h.AddItems(t, r) + } + + data := &Request{ + Log: h.log, + Restore: tc.restore, + Backup: tc.backup, + PodVolumeBackups: nil, + VolumeSnapshots: nil, + BackupReader: tc.tarball, + RestoredItems: map[itemKey]restoredItemStatus{}, + ResourceModifiers: tc.resourceModifiers, + } + warnings, errs := h.restorer.Restore( + data, + nil, // restoreItemActions + nil, // volume snapshotter getter + ) + + assertEmptyResults(t, warnings, errs) + assertRestoredItems(t, h, tc.want) + if len(tc.expectedRestoreItems) > 0 { + assert.EqualValues(t, tc.expectedRestoreItems, data.RestoredItems) + } + }) + } +} From e90ddd480956f1b6b04fe50516e4970466ef59ba Mon Sep 17 00:00:00 2001 From: Baudouin Herlicq Date: Fri, 16 Aug 2024 14:15:07 +0200 Subject: [PATCH 2/4] Update changelog file number Signed-off-by: Baudouin Herlicq --- changelogs/unreleased/{8116-BaudouinH => 8121-BaudouinH} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelogs/unreleased/{8116-BaudouinH => 8121-BaudouinH} (100%) diff --git a/changelogs/unreleased/8116-BaudouinH b/changelogs/unreleased/8121-BaudouinH similarity index 100% rename from changelogs/unreleased/8116-BaudouinH rename to changelogs/unreleased/8121-BaudouinH From b06f9b8d46d2ba278c188915c52ca6654a3d383f Mon Sep 17 00:00:00 2001 From: Baudouin Herlicq Date: Fri, 16 Aug 2024 14:19:24 +0200 Subject: [PATCH 3/4] Add documentation Signed-off-by: Baudouin Herlicq --- site/content/docs/main/restore-resource-modifiers.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/site/content/docs/main/restore-resource-modifiers.md b/site/content/docs/main/restore-resource-modifiers.md index 0c1f2f2172..ac4b317970 100644 --- a/site/content/docs/main/restore-resource-modifiers.md +++ b/site/content/docs/main/restore-resource-modifiers.md @@ -58,6 +58,11 @@ resourceModifierRules: - copy - test (covered below) +### Limitations of Resource Modifierss + +Resource modifiers cannot patch the status field of a resource. The patch will not be taken into account when doing so. +However, resource modifiers patch can use values from the status field of a resource to patch other fields. + ### Advanced scenarios #### **Conditional patches using test operation** The `test` operation can be used to check if a particular value is present in the resource. If the value is present, the patch will be applied. If the value is not present, the patch will not be applied. This can be used to apply a patch only if a particular value is present in the resource. For example, if you wish to change the storage class of a PVC only if the PVC is using a particular storage class, you can use the following configmap. From 662049d1e9c909802a68db7b6f016f0d77d848ce Mon Sep 17 00:00:00 2001 From: Baudouin Herlicq Date: Fri, 16 Aug 2024 14:20:12 +0200 Subject: [PATCH 4/4] Fix typo Signed-off-by: Baudouin Herlicq --- site/content/docs/main/restore-resource-modifiers.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/content/docs/main/restore-resource-modifiers.md b/site/content/docs/main/restore-resource-modifiers.md index ac4b317970..896550b898 100644 --- a/site/content/docs/main/restore-resource-modifiers.md +++ b/site/content/docs/main/restore-resource-modifiers.md @@ -58,7 +58,7 @@ resourceModifierRules: - copy - test (covered below) -### Limitations of Resource Modifierss +### Limitations of Resource Modifiers Resource modifiers cannot patch the status field of a resource. The patch will not be taken into account when doing so. However, resource modifiers patch can use values from the status field of a resource to patch other fields.