From 38c08e087bf8aa5b12ff0fc0984661e0e275ada5 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Mon, 8 Feb 2021 13:04:08 -0500 Subject: [PATCH] Replace NewObjectBackupStore with interface (#3329) In preparation for modifying the instantiation of `BackupStores` to be able to load credentials, change the function `NewObjectBackupStore` to be an interface that is passed in to all controllers. Previously, the function to get a new backup store was configurable but for many controllers was fixed to use `NewObjectBackupStore`. This change introduces an interface for getting the backup store and wraps the functionality from `NewObjectBackupStore` in a type which implements this interface. This will allow more flexibility when introducing credentials for a specific backup store as it will allow us to create a new `ObjectBackupStoreGetter` type which can be configured to add credentials config when creating the ObjectBackupStore without needing to change the API used by the controllers. Signed-off-by: Bridget McErlean --- pkg/cmd/server/server.go | 11 +++++-- pkg/controller/backup_controller.go | 9 ++--- pkg/controller/backup_controller_test.go | 8 ++--- pkg/controller/backup_deletion_controller.go | 9 ++--- .../backup_deletion_controller_test.go | 7 ++-- .../backup_storage_location_controller.go | 6 ++-- ...backup_storage_location_controller_test.go | 28 +++++----------- pkg/controller/backup_sync_controller.go | 9 ++--- pkg/controller/backup_sync_controller_test.go | 11 +++---- pkg/controller/download_request_controller.go | 9 ++--- .../download_request_controller_test.go | 6 +--- pkg/controller/restore_controller.go | 13 ++++---- pkg/controller/restore_controller_test.go | 11 +++---- pkg/controller/suite_test.go | 33 +++++++++++++++++++ pkg/persistence/object_store.go | 16 ++++++++- pkg/persistence/object_store_test.go | 11 ++++--- 16 files changed, 117 insertions(+), 80 deletions(-) diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index ce942ecf9..8f9451da3 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -578,6 +578,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string s.kubeClient, s.config.defaultBackupLocation, newPluginManager, + persistence.NewObjectBackupStoreGetter(), s.logger, ) @@ -620,6 +621,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string s.config.formatFlag.Parse(), csiVSLister, csiVSCLister, + persistence.NewObjectBackupStoreGetter(), ) return controllerRunInfo{ @@ -676,6 +678,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string csiVSCLister, s.csiSnapshotClient, newPluginManager, + persistence.NewObjectBackupStoreGetter(), s.metrics, s.discoveryHelper, ) @@ -713,6 +716,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string s.logger, s.logLevel, newPluginManager, + persistence.NewObjectBackupStoreGetter(), s.metrics, s.config.formatFlag.Parse(), ) @@ -747,6 +751,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string s.mgr.GetClient(), s.sharedInformerFactory.Velero().V1().Backups().Lister(), newPluginManager, + persistence.NewObjectBackupStoreGetter(), s.logger, ) @@ -824,9 +829,9 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string StorageLocation: s.config.defaultBackupLocation, ServerValidationFrequency: s.config.storeValidationFrequency, }, - NewPluginManager: newPluginManager, - NewBackupStore: persistence.NewObjectBackupStore, - Log: s.logger, + NewPluginManager: newPluginManager, + BackupStoreGetter: persistence.NewObjectBackupStoreGetter(), + Log: s.logger, } if err := bslr.SetupWithManager(s.mgr); err != nil { s.logger.Fatal(err, "unable to create controller", "controller", controller.BackupStorageLocation) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 05a28c29c..0da92ab9c 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -80,7 +80,7 @@ type backupController struct { snapshotLocationLister velerov1listers.VolumeSnapshotLocationLister defaultSnapshotLocations map[string]string metrics *metrics.ServerMetrics - newBackupStore func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) + backupStoreGetter persistence.ObjectBackupStoreGetter formatFlag logging.Format volumeSnapshotLister snapshotv1beta1listers.VolumeSnapshotLister volumeSnapshotContentLister snapshotv1beta1listers.VolumeSnapshotContentLister @@ -105,6 +105,7 @@ func NewBackupController( formatFlag logging.Format, volumeSnapshotLister snapshotv1beta1listers.VolumeSnapshotLister, volumeSnapshotContentLister snapshotv1beta1listers.VolumeSnapshotContentLister, + backupStoreGetter persistence.ObjectBackupStoreGetter, ) Interface { c := &backupController{ genericController: newGenericController(Backup, logger), @@ -126,7 +127,7 @@ func NewBackupController( formatFlag: formatFlag, volumeSnapshotLister: volumeSnapshotLister, volumeSnapshotContentLister: volumeSnapshotContentLister, - newBackupStore: persistence.NewObjectBackupStore, + backupStoreGetter: backupStoreGetter, } c.syncHandler = c.processBackup @@ -570,7 +571,7 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { } backupLog.Info("Setting up backup store to check for backup existence") - backupStore, err := c.newBackupStore(backup.StorageLocation, pluginManager, backupLog) + backupStore, err := c.backupStoreGetter.Get(backup.StorageLocation, pluginManager, backupLog) if err != nil { return err } @@ -651,7 +652,7 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error { // re-instantiate the backup store because credentials could have changed since the original // instantiation, if this was a long-running backup backupLog.Info("Setting up backup store to persist the backup") - backupStore, err = c.newBackupStore(backup.StorageLocation, pluginManager, backupLog) + backupStore, err = c.backupStoreGetter.Get(backup.StorageLocation, pluginManager, backupLog) if err != nil { return err } diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index e80aa0892..77564d2a8 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -818,11 +818,9 @@ func TestProcessBackupCompletions(t *testing.T) { metrics: metrics.NewServerMetrics(), clock: clock.NewFakeClock(now), newPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, - newBackupStore: func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) { - return backupStore, nil - }, - backupper: backupper, - formatFlag: formatFlag, + backupStoreGetter: NewFakeSingleObjectBackupStoreGetter(backupStore), + backupper: backupper, + formatFlag: formatFlag, } pluginManager.On("GetBackupItemActions").Return(nil, nil) diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index dc05ee257..3b85b4bb2 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -77,7 +77,7 @@ type backupDeletionController struct { processRequestFunc func(*velerov1api.DeleteBackupRequest) error clock clock.Clock newPluginManager func(logrus.FieldLogger) clientmgmt.Manager - newBackupStore func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) + backupStoreGetter persistence.ObjectBackupStoreGetter metrics *metrics.ServerMetrics helper discovery.Helper } @@ -99,6 +99,7 @@ func NewBackupDeletionController( csiSnapshotContentLister snapshotv1beta1listers.VolumeSnapshotContentLister, csiSnapshotClient *snapshotterClientSet.Clientset, newPluginManager func(logrus.FieldLogger) clientmgmt.Manager, + backupStoreGetter persistence.ObjectBackupStoreGetter, metrics *metrics.ServerMetrics, helper discovery.Helper, ) Interface { @@ -121,8 +122,8 @@ func NewBackupDeletionController( helper: helper, // use variables to refer to these functions so they can be // replaced with fakes for testing. - newPluginManager: newPluginManager, - newBackupStore: persistence.NewObjectBackupStore, + newPluginManager: newPluginManager, + backupStoreGetter: backupStoreGetter, clock: &clock.RealClock{}, } @@ -290,7 +291,7 @@ func (c *backupDeletionController) processRequest(req *velerov1api.DeleteBackupR pluginManager := c.newPluginManager(log) defer pluginManager.CleanupClients() - backupStore, err := c.newBackupStore(location, pluginManager, log) + backupStore, err := c.backupStoreGetter.Get(location, pluginManager, log) if err != nil { errs = append(errs, err.Error()) } diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index 3f58a562a..210e05a0b 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -75,6 +75,7 @@ func TestBackupDeletionControllerProcessQueueItem(t *testing.T) { nil, // csiSnapshotContentLister nil, // csiSnapshotClient nil, // new plugin manager func + persistence.NewObjectBackupStoreGetter(), metrics.NewServerMetrics(), nil, // discovery helper ).(*backupDeletionController) @@ -172,6 +173,7 @@ func setupBackupDeletionControllerTest(t *testing.T, objects ...runtime.Object) nil, // csiSnapshotContentLister nil, // csiSnapshotClient func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeSingleObjectBackupStoreGetter(backupStore), metrics.NewServerMetrics(), nil, // discovery helper ).(*backupDeletionController), @@ -179,10 +181,6 @@ func setupBackupDeletionControllerTest(t *testing.T, objects ...runtime.Object) req: req, } - data.controller.newBackupStore = func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) { - return backupStore, nil - } - pluginManager.On("CleanupClients").Return(nil) return data @@ -1133,6 +1131,7 @@ func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) { nil, // csiSnapshotContentLister nil, // csiSnapshotClient nil, // new plugin manager func + persistence.NewObjectBackupStoreGetter(), metrics.NewServerMetrics(), nil, // discovery helper, ).(*backupDeletionController) diff --git a/pkg/controller/backup_storage_location_controller.go b/pkg/controller/backup_storage_location_controller.go index d955ccd23..4f7d3dad9 100644 --- a/pkg/controller/backup_storage_location_controller.go +++ b/pkg/controller/backup_storage_location_controller.go @@ -44,8 +44,8 @@ type BackupStorageLocationReconciler struct { DefaultBackupLocationInfo storage.DefaultBackupLocationInfo // use variables to refer to these functions so they can be // replaced with fakes for testing. - NewPluginManager func(logrus.FieldLogger) clientmgmt.Manager - NewBackupStore func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) + NewPluginManager func(logrus.FieldLogger) clientmgmt.Manager + BackupStoreGetter persistence.ObjectBackupStoreGetter Log logrus.FieldLogger } @@ -95,7 +95,7 @@ func (r *BackupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr continue } - backupStore, err := r.NewBackupStore(location, pluginManager, log) + backupStore, err := r.BackupStoreGetter.Get(location, pluginManager, log) if err != nil { log.WithError(err).Error("Error getting a backup store") continue diff --git a/pkg/controller/backup_storage_location_controller_test.go b/pkg/controller/backup_storage_location_controller_test.go index b244f42cd..82a6cc241 100644 --- a/pkg/controller/backup_storage_location_controller_test.go +++ b/pkg/controller/backup_storage_location_controller_test.go @@ -33,7 +33,6 @@ import ( "github.com/vmware-tanzu/velero/internal/storage" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" - "github.com/vmware-tanzu/velero/pkg/persistence" persistencemocks "github.com/vmware-tanzu/velero/pkg/persistence/mocks" "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks" @@ -87,12 +86,9 @@ var _ = Describe("Backup Storage Location Reconciler", func() { StorageLocation: "location-1", ServerValidationFrequency: 0, }, - NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, - NewBackupStore: func(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) { - // this gets populated just below, prior to exercising the method under test - return backupStores[loc.Name], nil - }, - Log: velerotest.NewLogger(), + NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + BackupStoreGetter: NewFakeObjectBackupStoreGetter(backupStores), + Log: velerotest.NewLogger(), } actualResult, err := r.Reconcile(ctx, ctrl.Request{ @@ -156,12 +152,9 @@ var _ = Describe("Backup Storage Location Reconciler", func() { StorageLocation: "default", ServerValidationFrequency: 0, }, - NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, - NewBackupStore: func(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) { - // this gets populated just below, prior to exercising the method under test - return backupStores[loc.Name], nil - }, - Log: velerotest.NewLogger(), + NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + BackupStoreGetter: NewFakeObjectBackupStoreGetter(backupStores), + Log: velerotest.NewLogger(), } actualResult, err := r.Reconcile(ctx, ctrl.Request{ @@ -229,12 +222,9 @@ var _ = Describe("Backup Storage Location Reconciler", func() { StorageLocation: "default", ServerValidationFrequency: 0, }, - NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, - NewBackupStore: func(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) { - // this gets populated just below, prior to exercising the method under test - return backupStores[loc.Name], nil - }, - Log: velerotest.NewLogger(), + NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + BackupStoreGetter: NewFakeObjectBackupStoreGetter(backupStores), + Log: velerotest.NewLogger(), } actualResult, err := r.Reconcile(ctx, ctrl.Request{ diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index 75df92be2..e4c8ff022 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -54,7 +54,7 @@ type backupSyncController struct { defaultBackupLocation string defaultBackupSyncPeriod time.Duration newPluginManager func(logrus.FieldLogger) clientmgmt.Manager - newBackupStore func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) + backupStoreGetter persistence.ObjectBackupStoreGetter } func NewBackupSyncController( @@ -68,6 +68,7 @@ func NewBackupSyncController( kubeClient kubernetes.Interface, defaultBackupLocation string, newPluginManager func(logrus.FieldLogger) clientmgmt.Manager, + backupStoreGetter persistence.ObjectBackupStoreGetter, logger logrus.FieldLogger, ) Interface { if syncPeriod <= 0 { @@ -89,8 +90,8 @@ func NewBackupSyncController( // use variables to refer to these functions so they can be // replaced with fakes for testing. - newPluginManager: newPluginManager, - newBackupStore: persistence.NewObjectBackupStore, + newPluginManager: newPluginManager, + backupStoreGetter: backupStoreGetter, } c.resyncFunc = c.run @@ -169,7 +170,7 @@ func (c *backupSyncController) run() { log.Debug("Checking backup location for backups to sync into cluster") - backupStore, err := c.newBackupStore(&location, pluginManager, log) + backupStore, err := c.backupStoreGetter.Get(&location, pluginManager, log) if err != nil { log.WithError(err).Error("Error getting backup store for this location") continue diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index d7c52aecb..a55de5e0f 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -21,6 +21,8 @@ import ( "testing" "time" + "github.com/vmware-tanzu/velero/pkg/persistence" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,7 +36,6 @@ import ( "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/fake" informers "github.com/vmware-tanzu/velero/pkg/generated/informers/externalversions" "github.com/vmware-tanzu/velero/pkg/label" - "github.com/vmware-tanzu/velero/pkg/persistence" persistencemocks "github.com/vmware-tanzu/velero/pkg/persistence/mocks" "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks" @@ -351,14 +352,10 @@ func TestBackupSyncControllerRun(t *testing.T) { nil, // kubeClient "", func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeObjectBackupStoreGetter(backupStores), velerotest.NewLogger(), ).(*backupSyncController) - c.newBackupStore = func(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) { - // this gets populated just below, prior to exercising the method under test - return backupStores[loc.Name], nil - } - pluginManager.On("CleanupClients").Return(nil) for _, location := range test.locations { @@ -576,6 +573,7 @@ func TestDeleteOrphanedBackups(t *testing.T) { nil, // kubeClient "", nil, // new plugin manager func + persistence.NewObjectBackupStoreGetter(), velerotest.NewLogger(), ).(*backupSyncController) @@ -669,6 +667,7 @@ func TestStorageLabelsInDeleteOrphanedBackups(t *testing.T) { nil, // kubeClient "", nil, // new plugin manager func + persistence.NewObjectBackupStoreGetter(), velerotest.NewLogger(), ).(*backupSyncController) diff --git a/pkg/controller/download_request_controller.go b/pkg/controller/download_request_controller.go index 3ebd385ef..ea7d81599 100644 --- a/pkg/controller/download_request_controller.go +++ b/pkg/controller/download_request_controller.go @@ -51,7 +51,7 @@ type downloadRequestController struct { kbClient client.Client backupLister velerov1listers.BackupLister newPluginManager func(logrus.FieldLogger) clientmgmt.Manager - newBackupStore func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) + backupStoreGetter persistence.ObjectBackupStoreGetter } // NewDownloadRequestController creates a new DownloadRequestController. @@ -62,6 +62,7 @@ func NewDownloadRequestController( kbClient client.Client, backupLister velerov1listers.BackupLister, newPluginManager func(logrus.FieldLogger) clientmgmt.Manager, + backupStoreGetter persistence.ObjectBackupStoreGetter, logger logrus.FieldLogger, ) Interface { c := &downloadRequestController{ @@ -74,8 +75,8 @@ func NewDownloadRequestController( // use variables to refer to these functions so they can be // replaced with fakes for testing. - newPluginManager: newPluginManager, - newBackupStore: persistence.NewObjectBackupStore, + newPluginManager: newPluginManager, + backupStoreGetter: backupStoreGetter, clock: &clock.RealClock{}, } @@ -172,7 +173,7 @@ func (c *downloadRequestController) generatePreSignedURL(downloadRequest *velero pluginManager := c.newPluginManager(log) defer pluginManager.CleanupClients() - backupStore, err := c.newBackupStore(backupLocation, pluginManager, log) + backupStore, err := c.backupStoreGetter.Get(backupLocation, pluginManager, log) if err != nil { return errors.WithStack(err) } diff --git a/pkg/controller/download_request_controller_test.go b/pkg/controller/download_request_controller_test.go index 5f7d33ae2..21363e24c 100644 --- a/pkg/controller/download_request_controller_test.go +++ b/pkg/controller/download_request_controller_test.go @@ -34,7 +34,6 @@ import ( "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/fake" informers "github.com/vmware-tanzu/velero/pkg/generated/informers/externalversions" - "github.com/vmware-tanzu/velero/pkg/persistence" persistencemocks "github.com/vmware-tanzu/velero/pkg/persistence/mocks" "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks" @@ -64,6 +63,7 @@ func newDownloadRequestTestHarness(t *testing.T, fakeClient client.Client) *down fakeClient, informerFactory.Velero().V1().Backups().Lister(), func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeSingleObjectBackupStoreGetter(backupStore), velerotest.NewLogger(), ).(*downloadRequestController) ) @@ -72,10 +72,6 @@ func newDownloadRequestTestHarness(t *testing.T, fakeClient client.Client) *down require.NoError(t, err) controller.clock = clock.NewFakeClock(clockTime) - controller.newBackupStore = func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) { - return backupStore, nil - } - pluginManager.On("CleanupClients").Return() return &downloadRequestTestHarness{ diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index fe79f0083..cb015742e 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -92,8 +92,8 @@ type restoreController struct { logFormat logging.Format clock clock.Clock - newPluginManager func(logger logrus.FieldLogger) clientmgmt.Manager - newBackupStore func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) + newPluginManager func(logger logrus.FieldLogger) clientmgmt.Manager + backupStoreGetter persistence.ObjectBackupStoreGetter } func NewRestoreController( @@ -108,6 +108,7 @@ func NewRestoreController( logger logrus.FieldLogger, restoreLogLevel logrus.Level, newPluginManager func(logrus.FieldLogger) clientmgmt.Manager, + backupStoreGetter persistence.ObjectBackupStoreGetter, metrics *metrics.ServerMetrics, logFormat logging.Format, ) Interface { @@ -128,8 +129,8 @@ func NewRestoreController( // use variables to refer to these functions so they can be // replaced with fakes for testing. - newPluginManager: newPluginManager, - newBackupStore: persistence.NewObjectBackupStore, + newPluginManager: newPluginManager, + backupStoreGetter: backupStoreGetter, } c.syncHandler = c.processQueueItem @@ -410,7 +411,7 @@ func (c *restoreController) fetchBackupInfo(backupName string, pluginManager cli return backupInfo{}, errors.WithStack(err) } - backupStore, err := c.newBackupStore(location, pluginManager, c.logger) + backupStore, err := c.backupStoreGetter.Get(location, pluginManager, c.logger) if err != nil { return backupInfo{}, err } @@ -480,7 +481,7 @@ func (c *restoreController) runValidatedRestore(restore *api.Restore, info backu // re-instantiate the backup store because credentials could have changed since the original // instantiation, if this was a long-running restore - info.backupStore, err = c.newBackupStore(info.location, pluginManager, c.logger) + info.backupStore, err = c.backupStoreGetter.Get(info.location, pluginManager, c.logger) if err != nil { return errors.Wrap(err, "error setting up backup store to persist log and results files") } diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index f8069dce1..618c388ca 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -117,14 +117,11 @@ func TestFetchBackupInfo(t *testing.T) { logger, logrus.InfoLevel, func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeSingleObjectBackupStoreGetter(backupStore), metrics.NewServerMetrics(), formatFlag, ).(*restoreController) - c.newBackupStore = func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) { - return backupStore, nil - } - if test.backupStoreError == nil { for _, itm := range test.informerLocations { require.NoError(t, fakeClient.Create(context.Background(), itm)) @@ -212,6 +209,7 @@ func TestProcessQueueItemSkips(t *testing.T) { logger, logrus.InfoLevel, nil, + persistence.NewObjectBackupStoreGetter(), metrics.NewServerMetrics(), formatFlag, ).(*restoreController) @@ -438,13 +436,11 @@ func TestProcessQueueItem(t *testing.T) { logger, logrus.InfoLevel, func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeSingleObjectBackupStoreGetter(backupStore), metrics.NewServerMetrics(), formatFlag, ).(*restoreController) - c.newBackupStore = func(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) { - return backupStore, nil - } c.clock = clock.NewFakeClock(now) if test.location != nil { require.NoError(t, fakeClient.Create(context.Background(), test.location)) @@ -670,6 +666,7 @@ func TestvalidateAndCompleteWhenScheduleNameSpecified(t *testing.T) { logger, logrus.DebugLevel, nil, + persistence.NewObjectBackupStoreGetter(), nil, formatFlag, ).(*restoreController) diff --git a/pkg/controller/suite_test.go b/pkg/controller/suite_test.go index f0fb63307..007d38edb 100644 --- a/pkg/controller/suite_test.go +++ b/pkg/controller/suite_test.go @@ -22,6 +22,11 @@ import ( "testing" "time" + "github.com/sirupsen/logrus" + + "github.com/vmware-tanzu/velero/pkg/persistence" + persistencemocks "github.com/vmware-tanzu/velero/pkg/persistence/mocks" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -133,3 +138,31 @@ func newFakeClient(t *testing.T, initObjs ...runtime.Object) client.Client { require.NoError(t, err) return fake.NewFakeClientWithScheme(scheme.Scheme, initObjs...) } + +type fakeSingleObjectBackupStoreGetter struct { + store persistence.BackupStore +} + +func (f *fakeSingleObjectBackupStoreGetter) Get(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) { + return f.store, nil +} + +// NewFakeSingleObjectBackupStoreGetter returns an ObjectBackupStoreGetter +// that will return only the given BackupStore. +func NewFakeSingleObjectBackupStoreGetter(store persistence.BackupStore) persistence.ObjectBackupStoreGetter { + return &fakeSingleObjectBackupStoreGetter{store: store} +} + +type fakeObjectBackupStoreGetter struct { + stores map[string]*persistencemocks.BackupStore +} + +func (f *fakeObjectBackupStoreGetter) Get(loc *velerov1api.BackupStorageLocation, _ persistence.ObjectStoreGetter, _ logrus.FieldLogger) (persistence.BackupStore, error) { + return f.stores[loc.Name], nil +} + +// NewFakeObjectBackupStoreGetter returns an ObjectBackupStoreGetter that will +// return the BackupStore for a given BackupStorageLocation name. +func NewFakeObjectBackupStoreGetter(stores map[string]*persistencemocks.BackupStore) persistence.ObjectBackupStoreGetter { + return &fakeObjectBackupStoreGetter{stores: stores} +} diff --git a/pkg/persistence/object_store.go b/pkg/persistence/object_store.go index 20b7f6aad..4663ac6cc 100644 --- a/pkg/persistence/object_store.go +++ b/pkg/persistence/object_store.go @@ -90,7 +90,21 @@ type ObjectStoreGetter interface { GetObjectStore(provider string) (velero.ObjectStore, error) } -func NewObjectBackupStore(location *velerov1api.BackupStorageLocation, objectStoreGetter ObjectStoreGetter, logger logrus.FieldLogger) (BackupStore, error) { +// ObjectBackupStoreGetter is a type that can get a velero.BackupStore for a +// given BackupStorageLocation and ObjectStore. +type ObjectBackupStoreGetter interface { + Get(location *velerov1api.BackupStorageLocation, objectStoreGetter ObjectStoreGetter, logger logrus.FieldLogger) (BackupStore, error) +} + +type objectBackupStoreGetter struct{} + +// NewObjectBackupStoreGetter returns a ObjectBackupStoreGetter that can get a +// default velero.BackupStore. +func NewObjectBackupStoreGetter() ObjectBackupStoreGetter { + return &objectBackupStoreGetter{} +} + +func (b *objectBackupStoreGetter) Get(location *velerov1api.BackupStorageLocation, objectStoreGetter ObjectStoreGetter, logger logrus.FieldLogger) (BackupStore, error) { if location.Spec.ObjectStorage == nil { return nil, errors.New("backup storage location does not use object storage") } diff --git a/pkg/persistence/object_store_test.go b/pkg/persistence/object_store_test.go index 61d628c09..f019ff1f3 100644 --- a/pkg/persistence/object_store_test.go +++ b/pkg/persistence/object_store_test.go @@ -604,10 +604,10 @@ func (osg objectStoreGetter) GetObjectStore(provider string) (velero.ObjectStore return res, nil } -// TestNewObjectBackupStore runs the NewObjectBackupStore constructor and ensures -// that an ObjectBackupStore is constructed correctly or an appropriate error is -// returned. -func TestNewObjectBackupStore(t *testing.T) { +// TestNewObjectBackupStore runs the NewObjectBackupStoreGetter constructor and ensures +// that it provides a BackupStore with a correctly constructed ObjectBackupStore or +// that an appropriate error is returned. +func TestNewObjectBackupStoreGetter(t *testing.T) { tests := []struct { name string location *velerov1api.BackupStorageLocation @@ -661,7 +661,8 @@ func TestNewObjectBackupStore(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - res, err := NewObjectBackupStore(tc.location, tc.objectStoreGetter, velerotest.NewLogger()) + getter := NewObjectBackupStoreGetter() + res, err := getter.Get(tc.location, tc.objectStoreGetter, velerotest.NewLogger()) if tc.wantErr != "" { require.Equal(t, tc.wantErr, err.Error()) } else {