controllers: take a newPluginManager func in constructors

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

pkg/cmd/server/server.go

@@ -565,6 +565,10 @@ func (s *server) runControllers(config *api.Config, defaultBackupLocation *api.B
 	s.metrics = metrics.NewServerMetrics()
 	s.metrics.RegisterAllMetrics()
 
+	newPluginManager := func(logger logrus.FieldLogger) plugin.Manager {
+		return plugin.NewManager(logger, s.logLevel, s.pluginRegistry)
+	}
+
 	backupSyncController := controller.NewBackupSyncController(
 		s.arkClient.ArkV1(),
 		s.sharedInformerFactory.Ark().V1().Backups(),
@@ -572,9 +576,8 @@ func (s *server) runControllers(config *api.Config, defaultBackupLocation *api.B
 		s.config.backupSyncPeriod,
 		s.namespace,
 		s.config.defaultBackupLocation,
-		s.pluginRegistry,
+		newPluginManager,
 		s.logger,
-		s.logLevel,
 	)
 	wg.Add(1)
 	go func() {
@@ -604,7 +607,7 @@ func (s *server) runControllers(config *api.Config, defaultBackupLocation *api.B
 			s.blockStore != nil,
 			s.logger,
 			s.logLevel,
-			s.pluginRegistry,
+			newPluginManager,
 			backupTracker,
 			s.sharedInformerFactory.Ark().V1().BackupStorageLocations(),
 			s.config.defaultBackupLocation,
@@ -644,7 +647,6 @@ func (s *server) runControllers(config *api.Config, defaultBackupLocation *api.B
 
 		backupDeletionController := controller.NewBackupDeletionController(
 			s.logger,
-			s.logLevel,
 			s.sharedInformerFactory.Ark().V1().DeleteBackupRequests(),
 			s.arkClient.ArkV1(), // deleteBackupRequestClient
 			s.arkClient.ArkV1(), // backupClient
@@ -655,7 +657,7 @@ func (s *server) runControllers(config *api.Config, defaultBackupLocation *api.B
 			s.resticManager,
 			s.sharedInformerFactory.Ark().V1().PodVolumeBackups(),
 			s.sharedInformerFactory.Ark().V1().BackupStorageLocations(),
-			s.pluginRegistry,
+			newPluginManager,
 		)
 		wg.Add(1)
 		go func() {
@@ -689,7 +691,7 @@ func (s *server) runControllers(config *api.Config, defaultBackupLocation *api.B
 		s.blockStore != nil,
 		s.logger,
 		s.logLevel,
-		s.pluginRegistry,
+		newPluginManager,
 		s.config.defaultBackupLocation,
 		s.metrics,
 	)
@@ -706,9 +708,8 @@ func (s *server) runControllers(config *api.Config, defaultBackupLocation *api.B
 		s.sharedInformerFactory.Ark().V1().Restores(),
 		s.sharedInformerFactory.Ark().V1().BackupStorageLocations(),
 		s.sharedInformerFactory.Ark().V1().Backups(),
-		s.pluginRegistry,
+		newPluginManager,
 		s.logger,
-		s.logLevel,
 	)
 	wg.Add(1)
 	go func() {

pkg/controller/backup_controller.go

@@ -67,14 +67,12 @@ type backupController struct {
 	clock                      clock.Clock
 	logger                     logrus.FieldLogger
 	logLevel                   logrus.Level
-	pluginRegistry             plugin.Registry
+	newPluginManager           func(logrus.FieldLogger) plugin.Manager
 	backupTracker              BackupTracker
 	backupLocationLister       listers.BackupStorageLocationLister
 	backupLocationListerSynced cache.InformerSynced
 	defaultBackupLocation      string
 	metrics                    *metrics.ServerMetrics
-
-	newPluginManager func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager
 }
 
 func NewBackupController(
@@ -84,7 +82,7 @@ func NewBackupController(
 	pvProviderExists bool,
 	logger logrus.FieldLogger,
 	logLevel logrus.Level,
-	pluginRegistry plugin.Registry,
+	newPluginManager func(logrus.FieldLogger) plugin.Manager,
 	backupTracker BackupTracker,
 	backupLocationInformer informers.BackupStorageLocationInformer,
 	defaultBackupLocation string,
@@ -100,16 +98,12 @@ func NewBackupController(
 		clock:                      &clock.RealClock{},
 		logger:                     logger,
 		logLevel:                   logLevel,
-		pluginRegistry:             pluginRegistry,
+		newPluginManager:           newPluginManager,
 		backupTracker:              backupTracker,
 		backupLocationLister:       backupLocationInformer.Lister(),
 		backupLocationListerSynced: backupLocationInformer.Informer().HasSynced,
 		defaultBackupLocation:      defaultBackupLocation,
 		metrics:                    metrics,
-
-		newPluginManager: func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager {
-			return plugin.NewManager(logger, logLevel, pluginRegistry)
-		},
 	}
 
 	c.syncHandler = c.processBackup
@@ -387,7 +381,7 @@ func (controller *backupController) runBackup(backup *api.Backup, backupLocation
 
 	log.Info("Starting backup")
 
-	pluginManager := controller.newPluginManager(log, log.Level, controller.pluginRegistry)
+	pluginManager := controller.newPluginManager(log)
 	defer pluginManager.CleanupClients()
 
 	backupFile, err := ioutil.TempFile("", "")

pkg/controller/backup_controller_test.go

@@ -178,7 +178,6 @@ func TestProcessBackup(t *testing.T) {
 				backupper       = &fakeBackupper{}
 				sharedInformers = informers.NewSharedInformerFactory(client, 0)
 				logger          = logging.DefaultLogger(logrus.DebugLevel)
-				pluginRegistry  = plugin.NewRegistry("/dir", logger, logrus.InfoLevel)
 				clockTime, _    = time.Parse("Mon Jan 2 15:04:05 2006", "Mon Jan 2 15:04:05 2006")
 				objectStore     = &arktest.ObjectStore{}
 				pluginManager   = &pluginmocks.Manager{}
@@ -194,7 +193,7 @@ func TestProcessBackup(t *testing.T) {
 				test.allowSnapshots,
 				logger,
 				logrus.InfoLevel,
-				pluginRegistry,
+				func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 				NewBackupTracker(),
 				sharedInformers.Ark().V1().BackupStorageLocations(),
 				"default",
@@ -202,9 +201,6 @@ func TestProcessBackup(t *testing.T) {
 			).(*backupController)
 
 			c.clock = clock.NewFakeClock(clockTime)
-			c.newPluginManager = func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager {
-				return pluginManager
-			}
 
 			var expiration, startTime time.Time
 

pkg/controller/backup_deletion_controller.go

@@ -66,7 +66,6 @@ type backupDeletionController struct {
 // NewBackupDeletionController creates a new backup deletion controller.
 func NewBackupDeletionController(
 	logger logrus.FieldLogger,
-	logLevel logrus.Level,
 	deleteBackupRequestInformer informers.DeleteBackupRequestInformer,
 	deleteBackupRequestClient arkv1client.DeleteBackupRequestsGetter,
 	backupClient arkv1client.BackupsGetter,
@@ -77,7 +76,7 @@ func NewBackupDeletionController(
 	resticMgr restic.RepositoryManager,
 	podvolumeBackupInformer informers.PodVolumeBackupInformer,
 	backupLocationInformer informers.BackupStorageLocationInformer,
-	pluginRegistry plugin.Registry,
+	newPluginManager func(logrus.FieldLogger) plugin.Manager,
 ) Interface {
 	c := &backupDeletionController{
 		genericController:         newGenericController("backup-deletion", logger),
@@ -94,10 +93,8 @@ func NewBackupDeletionController(
 
 		// use variables to refer to these functions so they can be
 		// replaced with fakes for testing.
-		deleteBackupDir: cloudprovider.DeleteBackupDir,
-		newPluginManager: func(logger logrus.FieldLogger) plugin.Manager {
-			return plugin.NewManager(logger, logLevel, pluginRegistry)
-		},
+		newPluginManager: newPluginManager,
+		deleteBackupDir:  cloudprovider.DeleteBackupDir,
 
 		clock: &clock.RealClock{},
 	}

pkg/controller/backup_deletion_controller_test.go

@@ -48,7 +48,6 @@ func TestBackupDeletionControllerProcessQueueItem(t *testing.T) {
 
 	controller := NewBackupDeletionController(
 		arktest.NewLogger(),
-		logrus.InfoLevel,
 		sharedInformers.Ark().V1().DeleteBackupRequests(),
 		client.ArkV1(), // deleteBackupRequestClient
 		client.ArkV1(), // backupClient
@@ -59,7 +58,7 @@ func TestBackupDeletionControllerProcessQueueItem(t *testing.T) {
 		nil, // restic repository manager
 		sharedInformers.Ark().V1().PodVolumeBackups(),
 		sharedInformers.Ark().V1().BackupStorageLocations(),
-		nil, // pluginRegistry
+		nil, // new plugin manager func
 	).(*backupDeletionController)
 
 	// Error splitting key
@@ -135,7 +134,6 @@ func setupBackupDeletionControllerTest(objects ...runtime.Object) *backupDeletio
 		objectStore:     objectStore,
 		controller: NewBackupDeletionController(
 			arktest.NewLogger(),
-			logrus.InfoLevel,
 			sharedInformers.Ark().V1().DeleteBackupRequests(),
 			client.ArkV1(), // deleteBackupRequestClient
 			client.ArkV1(), // backupClient
@@ -146,14 +144,12 @@ func setupBackupDeletionControllerTest(objects ...runtime.Object) *backupDeletio
 			nil, // restic repository manager
 			sharedInformers.Ark().V1().PodVolumeBackups(),
 			sharedInformers.Ark().V1().BackupStorageLocations(),
-			nil, // pluginRegistry
+			func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 		).(*backupDeletionController),
 
 		req: req,
 	}
 
-	data.controller.newPluginManager = func(_ logrus.FieldLogger) plugin.Manager { return pluginManager }
-
 	pluginManager.On("GetObjectStore", "objStoreProvider").Return(objectStore, nil)
 	pluginManager.On("CleanupClients").Return(nil)
 
@@ -594,7 +590,6 @@ func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) {
 
 			controller := NewBackupDeletionController(
 				arktest.NewLogger(),
-				logrus.InfoLevel,
 				sharedInformers.Ark().V1().DeleteBackupRequests(),
 				client.ArkV1(), // deleteBackupRequestClient
 				client.ArkV1(), // backupClient
@@ -605,7 +600,7 @@ func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) {
 				nil,
 				sharedInformers.Ark().V1().PodVolumeBackups(),
 				sharedInformers.Ark().V1().BackupStorageLocations(),
-				nil, // pluginRegistry
+				nil, // new plugin manager func
 			).(*backupDeletionController)
 
 			fakeClock := &clock.FakeClock{}

pkg/controller/backup_sync_controller.go

@@ -57,9 +57,8 @@ func NewBackupSyncController(
 	syncPeriod time.Duration,
 	namespace string,
 	defaultBackupLocation string,
-	pluginRegistry plugin.Registry,
+	newPluginManager func(logrus.FieldLogger) plugin.Manager,
 	logger logrus.FieldLogger,
-	logLevel logrus.Level,
 ) Interface {
 	if syncPeriod < time.Minute {
 		logger.Infof("Provided backup sync period %v is too short. Setting to 1 minute", syncPeriod)
@@ -74,9 +73,9 @@ func NewBackupSyncController(
 		backupLister:                backupInformer.Lister(),
 		backupStorageLocationLister: backupStorageLocationInformer.Lister(),
 
-		newPluginManager: func(logger logrus.FieldLogger) plugin.Manager {
-			return plugin.NewManager(logger, logLevel, pluginRegistry)
-		},
+		// use variables to refer to these functions so they can be
+		// replaced with fakes for testing.
+		newPluginManager: newPluginManager,
 		listCloudBackups: cloudprovider.ListBackups,
 	}
 

pkg/controller/backup_sync_controller_test.go

@@ -177,14 +177,13 @@ func TestBackupSyncControllerRun(t *testing.T) {
 				time.Duration(0),
 				test.namespace,
 				"",
-				nil, // pluginRegistry
+				func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 				arktest.NewLogger(),
-				logrus.DebugLevel,
 			).(*backupSyncController)
 
-			c.newPluginManager = func(_ logrus.FieldLogger) plugin.Manager { return pluginManager }
 			pluginManager.On("GetObjectStore", "objStoreProvider").Return(objectStore, nil)
 			pluginManager.On("CleanupClients").Return(nil)
+
 			objectStore.On("Init", mock.Anything).Return(nil)
 
 			for _, location := range test.locations {
@@ -343,9 +342,8 @@ func TestDeleteOrphanedBackups(t *testing.T) {
 				time.Duration(0),
 				test.namespace,
 				"",
-				nil, // pluginRegistry
+				nil, // new plugin manager func
 				arktest.NewLogger(),
-				logrus.InfoLevel,
 			).(*backupSyncController)
 
 			expectedDeleteActions := make([]core.Action, 0)

pkg/controller/download_request_controller.go

@@ -60,9 +60,8 @@ func NewDownloadRequestController(
 	restoreInformer informers.RestoreInformer,
 	backupLocationInformer informers.BackupStorageLocationInformer,
 	backupInformer informers.BackupInformer,
-	pluginRegistry plugin.Registry,
+	newPluginManager func(logrus.FieldLogger) plugin.Manager,
 	logger logrus.FieldLogger,
-	logLevel logrus.Level,
 ) Interface {
 	c := &downloadRequestController{
 		genericController:     newGenericController("downloadrequest", logger),
@@ -74,10 +73,8 @@ func NewDownloadRequestController(
 
 		// use variables to refer to these functions so they can be
 		// replaced with fakes for testing.
-		createSignedURL: cloudprovider.CreateSignedURL,
-		newPluginManager: func(logger logrus.FieldLogger) plugin.Manager {
-			return plugin.NewManager(logger, logLevel, pluginRegistry)
-		},
+		createSignedURL:  cloudprovider.CreateSignedURL,
+		newPluginManager: newPluginManager,
 
 		clock: &clock.RealClock{},
 	}

pkg/controller/download_request_controller_test.go

@@ -59,9 +59,8 @@ func newDownloadRequestTestHarness(t *testing.T) *downloadRequestTestHarness {
 			informerFactory.Ark().V1().Restores(),
 			informerFactory.Ark().V1().BackupStorageLocations(),
 			informerFactory.Ark().V1().Backups(),
-			nil,
+			func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 			arktest.NewLogger(),
-			logrus.InfoLevel,
 		).(*downloadRequestController)
 	)
 
@@ -70,8 +69,6 @@ func newDownloadRequestTestHarness(t *testing.T) *downloadRequestTestHarness {
 
 	controller.clock = clock.NewFakeClock(clockTime)
 
-	controller.newPluginManager = func(_ logrus.FieldLogger) plugin.Manager { return pluginManager }
-
 	pluginManager.On("CleanupClients").Return()
 	objectStore.On("Init", mock.Anything).Return(nil)
 

pkg/controller/restore_controller.go

@@ -86,7 +86,6 @@ type restoreController struct {
 	queue                      workqueue.RateLimitingInterface
 	logger                     logrus.FieldLogger
 	logLevel                   logrus.Level
-	pluginRegistry             plugin.Registry
 	defaultBackupLocation      string
 	metrics                    *metrics.ServerMetrics
 
@@ -94,7 +93,7 @@ type restoreController struct {
 	downloadBackup       cloudprovider.DownloadBackupFunc
 	uploadRestoreLog     cloudprovider.UploadRestoreLogFunc
 	uploadRestoreResults cloudprovider.UploadRestoreResultsFunc
-	newPluginManager     func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager
+	newPluginManager     func(logger logrus.FieldLogger) plugin.Manager
 }
 
 func NewRestoreController(
@@ -108,10 +107,9 @@ func NewRestoreController(
 	pvProviderExists bool,
 	logger logrus.FieldLogger,
 	logLevel logrus.Level,
-	pluginRegistry plugin.Registry,
+	newPluginManager func(logrus.FieldLogger) plugin.Manager,
 	defaultBackupLocation string,
 	metrics *metrics.ServerMetrics,
-
 ) Interface {
 	c := &restoreController{
 		namespace:                  namespace,
@@ -128,17 +126,16 @@ func NewRestoreController(
 		queue:                 workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "restore"),
 		logger:                logger,
 		logLevel:              logLevel,
-		pluginRegistry:        pluginRegistry,
 		defaultBackupLocation: defaultBackupLocation,
 		metrics:               metrics,
 
+		// use variables to refer to these functions so they can be
+		// replaced with fakes for testing.
+		newPluginManager:     newPluginManager,
 		getBackup:            cloudprovider.GetBackup,
 		downloadBackup:       cloudprovider.DownloadBackup,
 		uploadRestoreLog:     cloudprovider.UploadRestoreLog,
 		uploadRestoreResults: cloudprovider.UploadRestoreResults,
-		newPluginManager: func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager {
-			return plugin.NewManager(logger, logLevel, pluginRegistry)
-		},
 	}
 
 	c.syncHandler = c.processRestore
@@ -282,7 +279,7 @@ func (c *restoreController) processRestore(key string) error {
 	// don't modify items in the cache
 	restore = restore.DeepCopy()
 
-	pluginManager := c.newPluginManager(logContext, logContext.Level, c.pluginRegistry)
+	pluginManager := c.newPluginManager(logContext)
 	defer pluginManager.CleanupClients()
 
 	actions, err := pluginManager.GetRestoreItemActions()

pkg/controller/restore_controller_test.go

@@ -105,13 +105,10 @@ func TestFetchBackupInfo(t *testing.T) {
 				false,
 				logger,
 				logrus.InfoLevel,
-				nil, //pluginRegistry
+				func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 				"default",
 				metrics.NewServerMetrics(),
 			).(*restoreController)
-			c.newPluginManager = func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager {
-				return pluginManager
-			}
 
 			if test.backupServiceError == nil {
 				pluginManager.On("GetObjectStore", "myCloud").Return(objectStore, nil)
@@ -200,13 +197,10 @@ func TestProcessRestoreSkips(t *testing.T) {
 				false, // pvProviderExists
 				logger,
 				logrus.InfoLevel,
-				nil, // pluginRegistry
+				func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 				"default",
 				metrics.NewServerMetrics(),
 			).(*restoreController)
-			c.newPluginManager = func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager {
-				return pluginManager
-			}
 
 			if test.restore != nil {
 				sharedInformers.Ark().V1().Restores().Informer().GetStore().Add(test.restore)
@@ -427,13 +421,10 @@ func TestProcessRestore(t *testing.T) {
 				test.allowRestoreSnapshots,
 				logger,
 				logrus.InfoLevel,
-				nil, // pluginRegistry
+				func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 				"default",
 				metrics.NewServerMetrics(),
 			).(*restoreController)
-			c.newPluginManager = func(logger logrus.FieldLogger, logLevel logrus.Level, pluginRegistry plugin.Registry) plugin.Manager {
-				return pluginManager
-			}
 
 			if test.location != nil {
 				sharedInformers.Ark().V1().BackupStorageLocations().Informer().GetStore().Add(test.location)