fix bug preventing backup item action item updates from saving

Signed-off-by: Steve Kriss <steve@heptio.com>

pkg/backup/item_backupper.go

@@ -204,11 +204,21 @@ func (ib *defaultItemBackupper) backupItem(logger logrus.FieldLogger, obj runtim
 		}
 	}
 
-	if err := ib.executeActions(log, obj, groupResource, name, namespace, metadata); err != nil {
+	updatedObj, err := ib.executeActions(log, obj, groupResource, name, namespace, metadata)
+	if err != nil {
 		log.WithError(err).Error("Error executing item actions")
 		backupErrs = append(backupErrs, err)
+
+		// if there was an error running actions, execute post hooks and return
+		log.Debug("Executing post hooks")
+		if err := ib.itemHookHandler.handleHooks(log, groupResource, obj, ib.resourceHooks, hookPhasePost); err != nil {
+			backupErrs = append(backupErrs, err)
 		}
 
+		return kubeerrs.NewAggregate(backupErrs)
+	}
+	obj = updatedObj
+
 	if groupResource == kuberesource.PersistentVolumes {
 		if ib.snapshotService == nil {
 			log.Debug("Skipping Persistent Volume snapshot because they're not enabled.")
@@ -285,7 +295,13 @@ func (ib *defaultItemBackupper) backupPodVolumes(log logrus.FieldLogger, pod *co
 	return ib.resticBackupper.BackupPodVolumes(ib.backup, pod, log)
 }
 
-func (ib *defaultItemBackupper) executeActions(log logrus.FieldLogger, obj runtime.Unstructured, groupResource schema.GroupResource, name, namespace string, metadata metav1.Object) error {
+func (ib *defaultItemBackupper) executeActions(
+	log logrus.FieldLogger,
+	obj runtime.Unstructured,
+	groupResource schema.GroupResource,
+	name, namespace string,
+	metadata metav1.Object,
+) (runtime.Unstructured, error) {
 	for _, action := range ib.actions {
 		if !action.resourceIncludesExcludes.ShouldInclude(groupResource.String()) {
 			log.Debug("Skipping action because it does not apply to this resource")
@@ -308,40 +324,40 @@ func (ib *defaultItemBackupper) executeActions(log logrus.FieldLogger, obj runti
 			logSetter.SetLog(log)
 		}
 
-		if updatedItem, additionalItemIdentifiers, err := action.Execute(obj, ib.backup); err == nil {
+		updatedItem, additionalItemIdentifiers, err := action.Execute(obj, ib.backup)
-			obj = updatedItem
-
-			for _, additionalItem := range additionalItemIdentifiers {
-				gvr, resource, err := ib.discoveryHelper.ResourceFor(additionalItem.GroupResource.WithVersion(""))
 		if err != nil {
-					return err
-				}
-
-				client, err := ib.dynamicFactory.ClientForGroupVersionResource(gvr.GroupVersion(), resource, additionalItem.Namespace)
-				if err != nil {
-					return err
-				}
-
-				additionalItem, err := client.Get(additionalItem.Name, metav1.GetOptions{})
-				if err != nil {
-					return err
-				}
-
-				if err = ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource()); err != nil {
-					return err
-				}
-			}
-		} else {
 			// We want this to show up in the log file at the place where the error occurs. When we return
 			// the error, it get aggregated with all the other ones at the end of the backup, making it
 			// harder to tell when it happened.
 			log.WithError(err).Error("error executing custom action")
 
-			return errors.Wrapf(err, "error executing custom action (groupResource=%s, namespace=%s, name=%s)", groupResource.String(), namespace, name)
+			return nil, errors.Wrapf(err, "error executing custom action (groupResource=%s, namespace=%s, name=%s)", groupResource.String(), namespace, name)
+		}
+		obj = updatedItem
+
+		for _, additionalItem := range additionalItemIdentifiers {
+			gvr, resource, err := ib.discoveryHelper.ResourceFor(additionalItem.GroupResource.WithVersion(""))
+			if err != nil {
+				return nil, err
+			}
+
+			client, err := ib.dynamicFactory.ClientForGroupVersionResource(gvr.GroupVersion(), resource, additionalItem.Namespace)
+			if err != nil {
+				return nil, err
+			}
+
+			additionalItem, err := client.Get(additionalItem.Name, metav1.GetOptions{})
+			if err != nil {
+				return nil, err
+			}
+
+			if err = ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource()); err != nil {
+				return nil, err
+			}
 		}
 	}
 
-	return nil
+	return obj, nil
 }
 
 // zoneLabel is the label that stores availability-zone info

pkg/backup/item_backupper_test.go

@@ -34,6 +34,7 @@ import (
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	corev1api "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -539,6 +540,84 @@ func TestBackupItemNoSkips(t *testing.T) {
 	}
 }
 
+type addAnnotationAction struct{}
+
+func (a *addAnnotationAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runtime.Unstructured, []ResourceIdentifier, error) {
+	// since item actions run out-of-proc, do a deep-copy here to simulate passing data
+	// across a process boundary.
+	copy := item.(*unstructured.Unstructured).DeepCopy()
+
+	metadata, err := meta.Accessor(copy)
+	if err != nil {
+		return copy, nil, nil
+	}
+
+	annotations := metadata.GetAnnotations()
+	if annotations == nil {
+		annotations = make(map[string]string)
+	}
+	annotations["foo"] = "bar"
+	metadata.SetAnnotations(annotations)
+
+	return copy, nil, nil
+}
+
+func (a *addAnnotationAction) AppliesTo() (ResourceSelector, error) {
+	panic("not implemented")
+}
+
+func TestItemActionModificationsToItemPersist(t *testing.T) {
+	var (
+		w   = &fakeTarWriter{}
+		obj = &unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"metadata": map[string]interface{}{
+					"namespace": "myns",
+					"name":      "bar",
+				},
+			},
+		}
+		actions = []resolvedAction{
+			{
+				ItemAction:                &addAnnotationAction{},
+				namespaceIncludesExcludes: collections.NewIncludesExcludes(),
+				resourceIncludesExcludes:  collections.NewIncludesExcludes(),
+				selector:                  labels.Everything(),
+			},
+		}
+		b = (&defaultItemBackupperFactory{}).newItemBackupper(
+			&v1.Backup{},
+			collections.NewIncludesExcludes(),
+			collections.NewIncludesExcludes(),
+			make(map[itemKey]struct{}),
+			actions,
+			nil,
+			w,
+			nil,
+			&arktest.FakeDynamicFactory{},
+			arktest.NewFakeDiscoveryHelper(true, nil),
+			nil,
+			nil,
+			newPVCSnapshotTracker(),
+		).(*defaultItemBackupper)
+	)
+
+	// our expected backed-up object is the passed-in object plus the annotation
+	// that the backup item action adds.
+	expected := obj.DeepCopy()
+	expected.SetAnnotations(map[string]string{"foo": "bar"})
+
+	// method under test
+	require.NoError(t, b.backupItem(arktest.NewLogger(), obj, schema.ParseGroupResource("resource.group")))
+
+	// get the actual backed-up item
+	require.Len(t, w.data, 1)
+	actual, err := arktest.GetAsMap(string(w.data[0]))
+	require.NoError(t, err)
+
+	assert.EqualValues(t, expected.Object, actual)
+}
+
 func TestTakePVSnapshot(t *testing.T) {
 	iops := int64(1000)