convert restorers to plugins

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

pkg/controller/backup_controller.go

@@ -320,7 +320,7 @@ func (controller *backupController) runBackup(backup *api.Backup, bucket string)
 		err = kuberrs.NewAggregate(errs)
 	}()
 
-	actions, err := controller.pluginManager.GetBackupItemActions(backup.Name, controller.logger, controller.logger.Level)
+	actions, err := controller.pluginManager.GetBackupItemActions(backup.Name)
 	if err != nil {
 		return err
 	}

pkg/controller/backup_controller_test.go

@@ -25,7 +25,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/clock"
 	core "k8s.io/client-go/testing"
 
-	"github.com/sirupsen/logrus"
 	testlogger "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -37,6 +36,7 @@ import (
 	"github.com/heptio/ark/pkg/generated/clientset/versioned/fake"
 	"github.com/heptio/ark/pkg/generated/clientset/versioned/scheme"
 	informers "github.com/heptio/ark/pkg/generated/informers/externalversions"
+	"github.com/heptio/ark/pkg/restore"
 	. "github.com/heptio/ark/pkg/util/test"
 )
 
@@ -49,94 +49,6 @@ func (b *fakeBackupper) Backup(backup *v1.Backup, data, log io.Writer, actions [
 	return args.Error(0)
 }
 
-// Manager is an autogenerated mock type for the Manager type
-type Manager struct {
-	mock.Mock
-}
-
-// CloseBackupItemActions provides a mock function with given fields: backupName
-func (_m *Manager) CloseBackupItemActions(backupName string) error {
-	ret := _m.Called(backupName)
-
-	var r0 error
-	if rf, ok := ret.Get(0).(func(string) error); ok {
-		r0 = rf(backupName)
-	} else {
-		r0 = ret.Error(0)
-	}
-
-	return r0
-}
-
-// GetBackupItemActions provides a mock function with given fields: backupName, logger, level
-func (_m *Manager) GetBackupItemActions(backupName string, logger logrus.FieldLogger, level logrus.Level) ([]backup.ItemAction, error) {
-	ret := _m.Called(backupName, logger, level)
-
-	var r0 []backup.ItemAction
-	if rf, ok := ret.Get(0).(func(string, logrus.FieldLogger, logrus.Level) []backup.ItemAction); ok {
-		r0 = rf(backupName, logger, level)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).([]backup.ItemAction)
-		}
-	}
-
-	var r1 error
-	if rf, ok := ret.Get(1).(func(string, logrus.FieldLogger, logrus.Level) error); ok {
-		r1 = rf(backupName, logger, level)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
-// GetBlockStore provides a mock function with given fields: name
-func (_m *Manager) GetBlockStore(name string) (cloudprovider.BlockStore, error) {
-	ret := _m.Called(name)
-
-	var r0 cloudprovider.BlockStore
-	if rf, ok := ret.Get(0).(func(string) cloudprovider.BlockStore); ok {
-		r0 = rf(name)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(cloudprovider.BlockStore)
-		}
-	}
-
-	var r1 error
-	if rf, ok := ret.Get(1).(func(string) error); ok {
-		r1 = rf(name)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
-// GetObjectStore provides a mock function with given fields: name
-func (_m *Manager) GetObjectStore(name string) (cloudprovider.ObjectStore, error) {
-	ret := _m.Called(name)
-
-	var r0 cloudprovider.ObjectStore
-	if rf, ok := ret.Get(0).(func(string) cloudprovider.ObjectStore); ok {
-		r0 = rf(name)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(cloudprovider.ObjectStore)
-		}
-	}
-
-	var r1 error
-	if rf, ok := ret.Get(1).(func(string) error); ok {
-		r1 = rf(name)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
 func TestProcessBackup(t *testing.T) {
 	tests := []struct {
 		name             string
@@ -243,7 +155,7 @@ func TestProcessBackup(t *testing.T) {
 				cloudBackups    = &BackupService{}
 				sharedInformers = informers.NewSharedInformerFactory(client, 0)
 				logger, _       = testlogger.NewNullLogger()
-				pluginManager   = &Manager{}
+				pluginManager   = &MockManager{}
 			)
 
 			c := NewBackupController(
@@ -284,7 +196,7 @@ func TestProcessBackup(t *testing.T) {
 
 				cloudBackups.On("UploadBackup", "bucket", backup.Name, mock.Anything, mock.Anything, mock.Anything).Return(nil)
 
-				pluginManager.On("GetBackupItemActions", backup.Name, logger, logger.Level).Return(nil, nil)
+				pluginManager.On("GetBackupItemActions", backup.Name).Return(nil, nil)
 				pluginManager.On("CloseBackupItemActions", backup.Name).Return(nil)
 			}
 
@@ -353,3 +265,128 @@ func TestProcessBackup(t *testing.T) {
 		})
 	}
 }
+
+// MockManager is an autogenerated mock type for the Manager type
+type MockManager struct {
+	mock.Mock
+}
+
+// CloseBackupItemActions provides a mock function with given fields: backupName
+func (_m *MockManager) CloseBackupItemActions(backupName string) error {
+	ret := _m.Called(backupName)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string) error); ok {
+		r0 = rf(backupName)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// GetBackupItemActions provides a mock function with given fields: backupName, logger, level
+func (_m *MockManager) GetBackupItemActions(backupName string) ([]backup.ItemAction, error) {
+	ret := _m.Called(backupName)
+
+	var r0 []backup.ItemAction
+	if rf, ok := ret.Get(0).(func(string) []backup.ItemAction); ok {
+		r0 = rf(backupName)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).([]backup.ItemAction)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(string) error); ok {
+		r1 = rf(backupName)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// CloseRestoreItemActions provides a mock function with given fields: restoreName
+func (_m *MockManager) CloseRestoreItemActions(restoreName string) error {
+	ret := _m.Called(restoreName)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string) error); ok {
+		r0 = rf(restoreName)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// GetRestoreItemActions provides a mock function with given fields: restoreName, logger, level
+func (_m *MockManager) GetRestoreItemActions(restoreName string) ([]restore.ItemAction, error) {
+	ret := _m.Called(restoreName)
+
+	var r0 []restore.ItemAction
+	if rf, ok := ret.Get(0).(func(string) []restore.ItemAction); ok {
+		r0 = rf(restoreName)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).([]restore.ItemAction)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(string) error); ok {
+		r1 = rf(restoreName)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// GetBlockStore provides a mock function with given fields: name
+func (_m *MockManager) GetBlockStore(name string) (cloudprovider.BlockStore, error) {
+	ret := _m.Called(name)
+
+	var r0 cloudprovider.BlockStore
+	if rf, ok := ret.Get(0).(func(string) cloudprovider.BlockStore); ok {
+		r0 = rf(name)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(cloudprovider.BlockStore)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(string) error); ok {
+		r1 = rf(name)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// GetObjectStore provides a mock function with given fields: name
+func (_m *MockManager) GetObjectStore(name string) (cloudprovider.ObjectStore, error) {
+	ret := _m.Called(name)
+
+	var r0 cloudprovider.ObjectStore
+	if rf, ok := ret.Get(0).(func(string) cloudprovider.ObjectStore); ok {
+		r0 = rf(name)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(cloudprovider.ObjectStore)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(string) error); ok {
+		r1 = rf(name)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}

pkg/controller/restore_controller.go

@@ -41,6 +41,7 @@ import (
 	arkv1client "github.com/heptio/ark/pkg/generated/clientset/versioned/typed/ark/v1"
 	informers "github.com/heptio/ark/pkg/generated/informers/externalversions/ark/v1"
 	listers "github.com/heptio/ark/pkg/generated/listers/ark/v1"
+	"github.com/heptio/ark/pkg/plugin"
 	"github.com/heptio/ark/pkg/restore"
 	"github.com/heptio/ark/pkg/util/collections"
 	kubeutil "github.com/heptio/ark/pkg/util/kube"
@@ -63,7 +64,8 @@ type restoreController struct {
 	restoreListerSynced cache.InformerSynced
 	syncHandler         func(restoreName string) error
 	queue               workqueue.RateLimitingInterface
-	logger              *logrus.Logger
+	logger              logrus.FieldLogger
+	pluginManager       plugin.Manager
 }
 
 func NewRestoreController(
@@ -75,7 +77,8 @@ func NewRestoreController(
 	bucket string,
 	backupInformer informers.BackupInformer,
 	pvProviderExists bool,
-	logger *logrus.Logger,
+	logger logrus.FieldLogger,
+	pluginManager plugin.Manager,
 ) Interface {
 	c := &restoreController{
 		restoreClient:       restoreClient,
@@ -90,6 +93,7 @@ func NewRestoreController(
 		restoreListerSynced: restoreInformer.Informer().HasSynced,
 		queue:               workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "restore"),
 		logger:              logger,
+		pluginManager:       pluginManager,
 	}
 
 	c.syncHandler = c.processRestore
@@ -391,8 +395,15 @@ func (controller *restoreController) runRestore(restore *api.Restore, bucket str
 		}
 	}()
 
+	actions, err := controller.pluginManager.GetRestoreItemActions(restore.Name)
+	if err != nil {
+		restoreErrors.Ark = append(restoreErrors.Ark, err.Error())
+		return
+	}
+	defer controller.pluginManager.CloseRestoreItemActions(restore.Name)
+
 	logContext.Info("starting restore")
-	restoreWarnings, restoreErrors = controller.restorer.Restore(restore, backup, backupFile, logFile)
+	restoreWarnings, restoreErrors = controller.restorer.Restore(restore, backup, backupFile, logFile, actions)
 	logContext.Info("restore completed")
 
 	// Try to upload the log file. This is best-effort. If we fail, we'll add to the ark errors.
@@ -431,7 +442,7 @@ func (controller *restoreController) runRestore(restore *api.Restore, bucket str
 	return
 }
 
-func downloadToTempFile(backupName string, backupService cloudprovider.BackupService, bucket string, logger *logrus.Logger) (*os.File, error) {
+func downloadToTempFile(backupName string, backupService cloudprovider.BackupService, bucket string, logger logrus.FieldLogger) (*os.File, error) {
 	readCloser, err := backupService.DownloadBackup(bucket, backupName)
 	if err != nil {
 		return nil, err

pkg/controller/restore_controller_test.go

@@ -23,7 +23,6 @@ import (
 	"io/ioutil"
 	"testing"
 
-	testlogger "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 
@@ -35,7 +34,8 @@ import (
 	api "github.com/heptio/ark/pkg/apis/ark/v1"
 	"github.com/heptio/ark/pkg/generated/clientset/versioned/fake"
 	informers "github.com/heptio/ark/pkg/generated/informers/externalversions"
-	. "github.com/heptio/ark/pkg/util/test"
+	"github.com/heptio/ark/pkg/restore"
+	arktest "github.com/heptio/ark/pkg/util/test"
 )
 
 func TestFetchBackup(t *testing.T) {
@@ -51,14 +51,14 @@ func TestFetchBackup(t *testing.T) {
 		{
 			name:            "lister has backup",
 			backupName:      "backup-1",
-			informerBackups: []*api.Backup{NewTestBackup().WithName("backup-1").Backup},
-			expectedRes:     NewTestBackup().WithName("backup-1").Backup,
+			informerBackups: []*api.Backup{arktest.NewTestBackup().WithName("backup-1").Backup},
+			expectedRes:     arktest.NewTestBackup().WithName("backup-1").Backup,
 		},
 		{
 			name:                "backupSvc has backup",
 			backupName:          "backup-1",
-			backupServiceBackup: NewTestBackup().WithName("backup-1").Backup,
-			expectedRes:         NewTestBackup().WithName("backup-1").Backup,
+			backupServiceBackup: arktest.NewTestBackup().WithName("backup-1").Backup,
+			expectedRes:         arktest.NewTestBackup().WithName("backup-1").Backup,
 		},
 		{
 			name:               "no backup",
@@ -74,8 +74,9 @@ func TestFetchBackup(t *testing.T) {
 				client          = fake.NewSimpleClientset()
 				restorer        = &fakeRestorer{}
 				sharedInformers = informers.NewSharedInformerFactory(client, 0)
-				backupSvc       = &BackupService{}
-				logger, _       = testlogger.NewNullLogger()
+				backupSvc       = &arktest.BackupService{}
+				logger          = arktest.NewLogger()
+				pluginManager   = &MockManager{}
 			)
 
 			c := NewRestoreController(
@@ -88,6 +89,7 @@ func TestFetchBackup(t *testing.T) {
 				sharedInformers.Ark().V1().Backups(),
 				false,
 				logger,
+				pluginManager,
 			).(*restoreController)
 
 			for _, itm := range test.informerBackups {
@@ -135,23 +137,23 @@ func TestProcessRestore(t *testing.T) {
 		},
 		{
 			name:        "restore with phase InProgress does not get processed",
-			restore:     NewTestRestore("foo", "bar", api.RestorePhaseInProgress).Restore,
+			restore:     arktest.NewTestRestore("foo", "bar", api.RestorePhaseInProgress).Restore,
 			expectedErr: false,
 		},
 		{
 			name:        "restore with phase Completed does not get processed",
-			restore:     NewTestRestore("foo", "bar", api.RestorePhaseCompleted).Restore,
+			restore:     arktest.NewTestRestore("foo", "bar", api.RestorePhaseCompleted).Restore,
 			expectedErr: false,
 		},
 		{
 			name:        "restore with phase FailedValidation does not get processed",
-			restore:     NewTestRestore("foo", "bar", api.RestorePhaseFailedValidation).Restore,
+			restore:     arktest.NewTestRestore("foo", "bar", api.RestorePhaseFailedValidation).Restore,
 			expectedErr: false,
 		},
 		{
 			name:        "restore with both namespace in both includedNamespaces and excludedNamespaces fails validation",
 			restore:     NewRestore("foo", "bar", "backup-1", "another-1", "*", api.RestorePhaseNew).WithExcludedNamespace("another-1").Restore,
-			backup:      NewTestBackup().WithName("backup-1").Backup,
+			backup:      arktest.NewTestBackup().WithName("backup-1").Backup,
 			expectedErr: false,
 			expectedRestoreUpdates: []*api.Restore{
 				NewRestore("foo", "bar", "backup-1", "another-1", "*", api.RestorePhaseFailedValidation).WithExcludedNamespace("another-1").
@@ -162,7 +164,7 @@ func TestProcessRestore(t *testing.T) {
 		{
 			name:        "restore with resource in both includedResources and excludedResources fails validation",
 			restore:     NewRestore("foo", "bar", "backup-1", "*", "a-resource", api.RestorePhaseNew).WithExcludedResource("a-resource").Restore,
-			backup:      NewTestBackup().WithName("backup-1").Backup,
+			backup:      arktest.NewTestBackup().WithName("backup-1").Backup,
 			expectedErr: false,
 			expectedRestoreUpdates: []*api.Restore{
 				NewRestore("foo", "bar", "backup-1", "*", "a-resource", api.RestorePhaseFailedValidation).WithExcludedResource("a-resource").
@@ -182,21 +184,21 @@ func TestProcessRestore(t *testing.T) {
 		},
 
 		{
-			name:                        "restore with non-existent backup name fails",
-			restore:                     NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore,
-			expectedErr:                 false,
-			backupServiceGetBackupError: errors.New("no backup here"),
+			name:        "restore with non-existent backup name fails",
+			restore:     arktest.NewTestRestore("foo", "bar", api.RestorePhaseNew).WithBackup("backup-1").WithIncludedNamespace("ns-1").Restore,
+			expectedErr: false,
 			expectedRestoreUpdates: []*api.Restore{
 				NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseInProgress).Restore,
 				NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseCompleted).
 					WithErrors(1).
 					Restore,
 			},
+			backupServiceGetBackupError: errors.New("no backup here"),
 		},
 		{
 			name:          "restorer throwing an error causes the restore to fail",
 			restore:       NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseNew).Restore,
-			backup:        NewTestBackup().WithName("backup-1").Backup,
+			backup:        arktest.NewTestBackup().WithName("backup-1").Backup,
 			restorerError: errors.New("blarg"),
 			expectedErr:   false,
 			expectedRestoreUpdates: []*api.Restore{
@@ -210,7 +212,7 @@ func TestProcessRestore(t *testing.T) {
 		{
 			name:        "valid restore gets executed",
 			restore:     NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseNew).Restore,
-			backup:      NewTestBackup().WithName("backup-1").Backup,
+			backup:      arktest.NewTestBackup().WithName("backup-1").Backup,
 			expectedErr: false,
 			expectedRestoreUpdates: []*api.Restore{
 				NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseInProgress).Restore,
@@ -221,7 +223,7 @@ func TestProcessRestore(t *testing.T) {
 		{
 			name:                  "valid restore with RestorePVs=true gets executed when allowRestoreSnapshots=true",
 			restore:               NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseNew).WithRestorePVs(true).Restore,
-			backup:                NewTestBackup().WithName("backup-1").Backup,
+			backup:                arktest.NewTestBackup().WithName("backup-1").Backup,
 			allowRestoreSnapshots: true,
 			expectedErr:           false,
 			expectedRestoreUpdates: []*api.Restore{
@@ -233,7 +235,7 @@ func TestProcessRestore(t *testing.T) {
 		{
 			name:        "restore with RestorePVs=true fails validation when allowRestoreSnapshots=false",
 			restore:     NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseNew).WithRestorePVs(true).Restore,
-			backup:      NewTestBackup().WithName("backup-1").Backup,
+			backup:      arktest.NewTestBackup().WithName("backup-1").Backup,
 			expectedErr: false,
 			expectedRestoreUpdates: []*api.Restore{
 				NewRestore("foo", "bar", "backup-1", "ns-1", "", api.RestorePhaseFailedValidation).
@@ -245,7 +247,7 @@ func TestProcessRestore(t *testing.T) {
 		{
 			name:        "restoration of nodes is not supported",
 			restore:     NewRestore("foo", "bar", "backup-1", "ns-1", "nodes", api.RestorePhaseNew).Restore,
-			backup:      NewTestBackup().WithName("backup-1").Backup,
+			backup:      arktest.NewTestBackup().WithName("backup-1").Backup,
 			expectedErr: false,
 			expectedRestoreUpdates: []*api.Restore{
 				NewRestore("foo", "bar", "backup-1", "ns-1", "nodes", api.RestorePhaseFailedValidation).
@@ -262,8 +264,9 @@ func TestProcessRestore(t *testing.T) {
 				client          = fake.NewSimpleClientset()
 				restorer        = &fakeRestorer{}
 				sharedInformers = informers.NewSharedInformerFactory(client, 0)
-				backupSvc       = &BackupService{}
-				logger, _       = testlogger.NewNullLogger()
+				backupSvc       = &arktest.BackupService{}
+				logger          = arktest.NewLogger()
+				pluginManager   = &MockManager{}
 			)
 
 			defer restorer.AssertExpectations(t)
@@ -279,6 +282,7 @@ func TestProcessRestore(t *testing.T) {
 				sharedInformers.Ark().V1().Backups(),
 				test.allowRestoreSnapshots,
 				logger,
+				pluginManager,
 			).(*restoreController)
 
 			if test.restore != nil {
@@ -331,6 +335,11 @@ func TestProcessRestore(t *testing.T) {
 				backupSvc.On("GetBackup", "bucket", test.restore.Spec.BackupName).Return(nil, test.backupServiceGetBackupError)
 			}
 
+			if test.restore != nil {
+				pluginManager.On("GetRestoreItemActions", test.restore.Name).Return(nil, nil)
+				pluginManager.On("CloseRestoreItemActions", test.restore.Name).Return(nil)
+			}
+
 			err = c.processRestore(key)
 			backupSvc.AssertExpectations(t)
 			restorer.AssertExpectations(t)
@@ -367,8 +376,8 @@ func TestProcessRestore(t *testing.T) {
 	}
 }
 
-func NewRestore(ns, name, backup, includeNS, includeResource string, phase api.RestorePhase) *TestRestore {
-	restore := NewTestRestore(ns, name, phase).WithBackup(backup)
+func NewRestore(ns, name, backup, includeNS, includeResource string, phase api.RestorePhase) *arktest.TestRestore {
+	restore := arktest.NewTestRestore(ns, name, phase).WithBackup(backup)
 
 	if includeNS != "" {
 		restore = restore.WithIncludedNamespace(includeNS)
@@ -390,7 +399,13 @@ type fakeRestorer struct {
 	calledWithArg api.Restore
 }
 
-func (r *fakeRestorer) Restore(restore *api.Restore, backup *api.Backup, backupReader io.Reader, logger io.Writer) (api.RestoreResult, api.RestoreResult) {
+func (r *fakeRestorer) Restore(
+	restore *api.Restore,
+	backup *api.Backup,
+	backupReader io.Reader,
+	logger io.Writer,
+	actions []restore.ItemAction,
+) (api.RestoreResult, api.RestoreResult) {
 	res := r.Called(restore, backup, backupReader, logger)
 
 	r.calledWithArg = *restore