Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update restore logic to allow Resource Modifiers to use data from the status field #8121

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Update restore logic to allow Resource Modifiers to use data from the status field #8121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9c7541
Update restore logic to allow Resource Modifiers to use data from the…
BaudouinH Aug 16, 2024
e90ddd4
Update changelog file number
BaudouinH Aug 16, 2024
b06f9b8
Add documentation
BaudouinH Aug 16, 2024
662049d
Fix typo
BaudouinH Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/unreleased/8121-BaudouinH
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Change restore flow to allow resourceModifiers to use status field in their patches.
18 changes: 9 additions & 9 deletions pkg/restore/restore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,15 +1294,6 @@
}
}

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the concern that @reasonerjt had in #8094 (comment) still holds.
Please pitch in others - CC: @sseago, if this could impact any RIA flows.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise please signoff - @reasonerjt

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anshulahuja98 I'm unaware of any RIA that currently modifies status -- if I'm remembering correctly, don't we currently strip the status field before RIAs are run? I think that's one of the reasons we added unmodified item (itemFromBackup) to the RIA input, as it still has the full status available for plugins to reference.

for _, action := range ctx.getApplicableActions(groupResource, namespace) {
if !action.Selector.Matches(labels.Set(obj.GetLabels())) {
continue
Expand Down Expand Up @@ -1451,6 +1442,15 @@
}
}

// 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

Check warning on line 1450 in pkg/restore/restore.go

View check run for this annotation

Codecov / codecov/patch

pkg/restore/restore.go#L1449-L1450 

Added lines #L1449 - L1450 were not covered by tests
}
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()
Expand Down
170 changes: 170 additions & 0 deletions pkg/restore/restore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
})
}
}
5 changes: 5 additions & 0 deletions site/content/docs/main/restore-resource-modifiers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ resourceModifierRules:
- copy
- test (covered below)

### Limitations of Resource Modifiers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please move limitations section to the bottom of the docs?


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.
Expand Down
Loading