Add regression test for additional item with invalid JSON (#10103)

archive.Unmarshal returns (nil, err) when an item file contains malformed
JSON, and restoreItem dereferences its obj argument on entry, so an
additional item that fails to unmarshal must be skipped rather than passed
on. The loop only records the error and continues today; nothing covers
that, so removing the continue reintroduces a nil pointer dereference in
the restore reconciler without failing any test.

The item file is added to the tarball so the existing Stat check passes and
the unmarshal is actually reached.

Signed-off-by: chlins <chlins.zhang@gmail.com>
This commit is contained in:
Chlins Zhang
2026-08-04 09:50:37 -04:00
committed by GitHub
parent b8db629c26
commit b4b72a3562
+56
View File
@@ -2299,6 +2299,62 @@ func TestRestoreActionAdditionalItems(t *testing.T) {
}
}
// TestRestoreActionAdditionalItemsInvalidJSON verifies that an additional item whose file
// exists in the backup but does not contain valid JSON is reported as an error and skipped,
// rather than being passed to restoreItem as a nil object.
//
// archive.Unmarshal returns (nil, err) for malformed JSON, and restoreItem dereferences its
// obj argument immediately, so failing to skip the item panics the restore reconciler.
func TestRestoreActionAdditionalItemsInvalidJSON(t *testing.T) {
h := newHarness(t)
for _, r := range []*test.APIResource{test.Pods(), test.PVs()} {
h.AddItems(t, r)
}
// pv-1.json exists so the Stat check passes, but its contents are not valid JSON.
tarball := test.NewTarWriter(t).
AddItems("pods", builder.ForPod("ns-1", "pod-1").Result()).
Add("resources/persistentvolumes/cluster/pv-1.json", []byte("not-json")).
Done()
actions := []riav2.RestoreItemAction{
&pluggableAction{
executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) {
return &velero.RestoreItemActionExecuteOutput{
UpdatedItem: input.Item,
AdditionalItems: []velero.ResourceIdentifier{
{GroupResource: kuberesource.PersistentVolumes, Name: "pv-1"},
},
}, nil
},
},
}
data := &Request{
Log: h.log,
Restore: defaultRestore().Result(),
Backup: defaultBackup().Result(),
BackupReader: tarball,
}
// A nil additional item passed on to restoreItem panics here rather than failing.
warnings, errs := h.restorer.Restore(data, actions, nil)
assertWantErrsOrWarnings(t, Result{}, warnings)
assertWantErrsOrWarnings(t, Result{
Namespaces: map[string][]string{
"ns-1": {"error restoring additional item persistentvolumes/pv-1"},
},
}, errs)
// The item that triggered the action is still restored, so the loop continued.
assertAPIContents(t, h, map[*test.APIResource][]string{
test.Pods(): {"ns-1/pod-1"},
test.PVs(): {},
})
}
// TestRestoreMustIncludeAdditionalItems covers restore must-include edge cases beyond the
// basic filter-bypass cases in TestRestoreActionAdditionalItems.
func TestRestoreMustIncludeAdditionalItems(t *testing.T) {